[llvm-lit] Adding -e option to lit's built-in cat command #102061

connieyzhu · 2024-08-05T21:12:48Z

This patch extends support for using cat -e in lit tests that use the internal
shell. The logic for enabling options is changed to accommodate for the
possibility of having several options. This patch is dependent on #101530.

Fixes #102377

This patch extends support for using cat -e in lit tests that use the internal shell. The logic for enabling options is changed to accommodate for the possibility of having several options.

This patch changes the storing of enabled options from a list of bools to a dataclass for better efficiency when accessing and checking the options.

llvmbot · 2024-08-05T21:13:22Z

@llvm/pr-subscribers-testing-tools

Author: Connie (connieyzhu)

Changes

This patch extends support for using cat -e in lit tests that use the internal
shell. The logic for enabling options is changed to accommodate for the
possibility of having several options. This patch is dependent on #101530.

Full diff: https://github.com/llvm/llvm-project/pull/102061.diff

1 Files Affected:

(modified) llvm/utils/lit/lit/builtin_commands/cat.py (+34-19)

diff --git a/llvm/utils/lit/lit/builtin_commands/cat.py b/llvm/utils/lit/lit/builtin_commands/cat.py
index d6dbe889cc2b1..14056e3ccd444 100644
--- a/llvm/utils/lit/lit/builtin_commands/cat.py
+++ b/llvm/utils/lit/lit/builtin_commands/cat.py
@@ -1,29 +1,42 @@
 import getopt
 import sys
+from dataclasses import dataclass, fields
 
 try:
     from StringIO import StringIO
 except ImportError:
     from io import StringIO
 
+@dataclass
+class Options:
+    show_ends: bool
+    show_nonprinting: bool
 
-def convertToCaretAndMNotation(data):
+def convertTextNotation(data, options):
     newdata = StringIO()
     if isinstance(data, str):
         data = bytearray(data.encode())
-
+    
     for intval in data:
-        if intval == 9 or intval == 10:
-            newdata.write(chr(intval))
-            continue
-        if intval > 127:
-            intval = intval - 128
-            newdata.write("M-")
-        if intval < 32:
-            newdata.write("^")
-            newdata.write(chr(intval + 64))
-        elif intval == 127:
-            newdata.write("^?")
+        if options.show_ends:
+            if intval == 10:
+                newdata.write("$")
+                newdata.write(chr(intval))
+                continue
+        if options.show_nonprinting:
+            if intval == 9 or intval == 10:
+                newdata.write(chr(intval))
+                continue
+            if intval > 127:
+                intval = intval - 128
+                newdata.write("M-")
+            if intval < 32:
+                newdata.write("^")
+                newdata.write(chr(intval + 64))
+            elif intval == 127:
+                newdata.write("^?")
+            else:
+                newdata.write(chr(intval))
         else:
             newdata.write(chr(intval))
 
@@ -32,9 +45,9 @@ def convertToCaretAndMNotation(data):
 
 def main(argv):
     arguments = argv[1:]
-    short_options = "v"
+    short_options = "ve"
     long_options = ["show-nonprinting"]
-    show_nonprinting = False
+    enabled_options = Options(show_ends=False, show_nonprinting=False)
 
     try:
         options, filenames = getopt.gnu_getopt(arguments, short_options, long_options)
@@ -43,8 +56,10 @@ def main(argv):
         sys.exit(1)
 
     for option, value in options:
-        if option == "-v" or option == "--show-nonprinting":
-            show_nonprinting = True
+        if option == "-v" or option == "--show-nonprinting" or option == "-e":
+            enabled_options.show_nonprinting = True
+        if option == "-e":
+            enabled_options.show_ends = True
 
     writer = getattr(sys.stdout, "buffer", None)
     if writer is None:
@@ -69,8 +84,8 @@ def main(argv):
                 fileToCat = open(filename, "rb")
                 contents = fileToCat.read()
 
-            if show_nonprinting:
-                contents = convertToCaretAndMNotation(contents)
+            if True in fields(enabled_options):
+                contents = convertTextNotation(contents, enabled_options)
             elif is_text:
                 contents = contents.encode()
             writer.write(contents)

connieyzhu · 2024-08-05T21:14:33Z

CC: @ilovepi @petrhosek

github-actions · 2024-08-05T21:17:28Z

✅ With the latest revision this PR passed the Python code formatter.

ilovepi

Mostly OK, with some minor comments, but don't the tests from #101530 need to be updated in this patch?

ilovepi · 2024-08-05T21:52:45Z

llvm/utils/lit/lit/builtin_commands/cat.py

+@dataclass
+class Options:
+    show_ends: bool
+    show_nonprinting: bool


Maybe some comment/documentation on this classes intended usage? probably at least worth doing for the fields.

ilovepi · 2024-08-05T22:00:04Z

llvm/utils/lit/lit/builtin_commands/cat.py

+            if True in fields(enabled_options):
+                contents = convertTextNotation(contents, enabled_options)


What happens if I add a field to Options that isn't either show_nonprinting or show_ends?

If both show_nonprinting and show_ends are False and this added field was True, convertTextNotation has an else statement on lines 43/44 that should account for this case, and not change the text (assuming that there is no existing implementation for this new option). But this is just a precaution, because I don't think this case should technically ever occur though - any option that exists in Options should have an implementation in convertTextNotation. Does this address your concern or am I misunderstanding?

But that means well run the conversion code when we don't need to right? It's preferable in my opinion to avoid that work if we don't have to, specially since that routine a character by character copy.

I'm also wondering if we should have an early exit from the routine if the neither option is set. WDYT?

ilovepi · 2024-08-05T22:41:52Z

llvm/utils/lit/lit/builtin_commands/cat.py

+        if options.show_ends:
+            if intval == 10:


nit: you can combine these and drop a level of nesting. Could also be if/else if/else but that isn't too important since there's a continue.

connieyzhu · 2024-08-05T23:18:00Z

Mostly OK, with some minor comments, but don't the tests from #101530 need to be updated in this patch?

@ilovepi I wouldn't be able to update the tests in this patch until #101530 has landed right, since I would need to rebase first?

logic

ilovepi

Mostly OK, with some minor comments, but don't the tests from #101530 need to be updated in this patch?

ilovepi · 2024-08-06T17:11:26Z

llvm/utils/lit/lit/builtin_commands/cat.py

+            if True in fields(enabled_options):
+                contents = convertTextNotation(contents, enabled_options)


But that means well run the conversion code when we don't need to right? It's preferable in my opinion to avoid that work if we don't have to, specially since that routine a character by character copy.

I'm also wondering if we should have an early exit from the routine if the neither option is set. WDYT?

This patch adds a new bool variable convert_text that is turned on if show_nonprinting, show_ends, or any option added in the future that is meant to modify text is True. This fixes the faulty logic that assumes that any option that is True is one that will modify text. Now, convertTextNotation() will only be called for options that need it.

jh7370

Change looks basically fine to me (once tests are updated). One question I do have is how many tests actually use this functionality and do they really need to?

connieyzhu · 2024-08-07T17:10:48Z

One question I do have is how many tests actually use this functionality and do they really need to?

@jh7370 I believe only one test currently uses cat -e. I'm not quite sure what its purpose is, but it's in TestUseColor.test in lldb.

This patch adds an assert to guarantee that when calling convertTextNotation(), the show_ends and show_nonprinting options are turned on. Some logic is also simplified when converting characters.

This patch allows for lit's built-in cat to use the -E option. The functionality was already implemented in previous patches, but this patch allows for the parsing and usage of -E.

jh7370 · 2024-08-08T08:34:22Z

One question I do have is how many tests actually use this functionality and do they really need to?

@jh7370 I believe only one test currently uses cat -e. I'm not quite sure what its purpose is, but it's in TestUseColor.test in lldb.

Thanks, I don't see the need for the $ line ending behaviour in that test. It's possible the non-printing characters behaviour is needed though. @JDevlieghere, since you last touched that test, can you confirm the purpose of the -e usage is to print non-printing characters?

We should avoid adding functionality that isn't needed, so for example, -v might be more appropriate in that test, which would then allow us to avoid adding -e functionality.

petrhosek · 2024-08-19T22:14:17Z

If we decide to go ahead with this change, I'd consider replacing the use of getopt with argparse which is more idiomatic in modern Python and should simplify the implementation.

…ernal shell (#104878) This patch changes the test that uses the `cat -e` option to `cat -v` so that the test can be run using lit's internal shell. For `cat`, the `-v` option prints non-printing characters in ^ and M- notation, while the `-e` option adds `$` to the end of lines in addition to printing non-printing characters in ^ and M- notation. This is an alternative patch to #102061, opting to rewrite the test that uses `cat -e` instead of extending support to the `-e` option. Fixes #102377

connieyzhu · 2024-08-21T00:03:03Z

Decided to go with the solution in #104878, so I will be closing this PR.

connieyzhu added 2 commits August 5, 2024 17:47

[llvm-lit] Adding -e option to lit's built-in cat command

bfad1ed

This patch extends support for using cat -e in lit tests that use the internal shell. The logic for enabling options is changed to accommodate for the possibility of having several options.

[llvm-lit] Restructure the tracking of enabled cat options

1d41bfa

This patch changes the storing of enabled options from a list of bools to a dataclass for better efficiency when accessing and checking the options.

llvmbot added llvm-lit testing-tools labels Aug 5, 2024

[llvm-lit] Fixed darker formatting issue

9eef7a5

ilovepi requested review from ilovepi and petrhosek August 5, 2024 21:46

ilovepi reviewed Aug 5, 2024

View reviewed changes

[llvm-lit][NFC] Added comments for dataclass + simplified show_ends

fcc87bc

logic

ilovepi reviewed Aug 6, 2024

View reviewed changes

jh7370 reviewed Aug 7, 2024

View reviewed changes

connieyzhu added 2 commits August 7, 2024 17:15

[llvm-lit] Added extra check when calling convertTextNotation

fcf072a

This patch adds an assert to guarantee that when calling convertTextNotation(), the show_ends and show_nonprinting options are turned on. Some logic is also simplified when converting characters.

[llvm-lit] Added -E option to built-in cat

a44eecb

This patch allows for lit's built-in cat to use the -E option. The functionality was already implemented in previous patches, but this patch allows for the parsing and usage of -E.

connieyzhu mentioned this pull request Aug 7, 2024

[llvm-lit][test] Precommit tests for lit's built-in cat command #101530

Closed

connieyzhu mentioned this pull request Aug 19, 2024

[lldb][test] Change unsupported cat -e to cat -v to work with lit internal shell #104878

Merged

connieyzhu closed this Aug 21, 2024

connieyzhu deleted the lldb-cat branch August 22, 2024 17:28

		if True in fields(enabled_options):
		contents = convertTextNotation(contents, enabled_options)

[llvm-lit] Adding -e option to lit's built-in cat command #102061

[llvm-lit] Adding -e option to lit's built-in cat command #102061

Uh oh!

Conversation

connieyzhu commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 5, 2024

Uh oh!

connieyzhu commented Aug 5, 2024

Uh oh!

github-actions bot commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connieyzhu commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

connieyzhu commented Aug 7, 2024

Uh oh!

jh7370 commented Aug 8, 2024

Uh oh!

petrhosek commented Aug 19, 2024

Uh oh!

connieyzhu commented Aug 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

connieyzhu commented Aug 5, 2024 •

edited

Loading

github-actions bot commented Aug 5, 2024 •

edited

Loading

connieyzhu commented Aug 5, 2024 •

edited

Loading