[PATCH] D66015: [llvm-strings] Improve testing of llvm-strings

Mon Aug 12 03:07:24 PDT 2019

jhenderson marked 10 inline comments as done.
jhenderson added inline comments.

================
Comment at: test/tools/llvm-strings/all-sections.test:1
-RUN: llvm-strings -a %S/Inputs/abcd | FileCheck %s
-RUN: llvm-strings --all %S/Inputs/abcd | FileCheck %s
+# Show that the -a/--all switch is accepted. GNU strings checks specific object
+# file sections unless --all is specified. llvm-strings just looks at the entire
----------------
grimar wrote:
> Should we use `##` in this tests too? I would do this, since it is probably
> not bad to have a single style across the all LLVM tests.
I didn't bother here because this isn't used as an input to anything, but I can do so if you're bothered. Should I add '#' to the RUN and CHECK lines too? I'd prefer not to, mostly because that will unnecessarily make git blame less useful.

================
Comment at: test/tools/llvm-strings/eof-no-string.test:1
+# Show that llvm-strings does not print the last string in the input if it is
+# too short and no unprintable character follows it.
----------------
I'm not sure why my patch isn't showing this, but this has been renamed from "terminator-neg.test".

================
Comment at: test/tools/llvm-strings/eof-no-string.test:2
+# Show that llvm-strings does not print the last string in the input if it is
+# too short and no unprintable character follows it.
+
----------------
grimar wrote:
> I think it just does not print all strings that are shorter than 4 bytes?
> I.e. not only the last line.
Right, but this is specifically about the last line case (there are other cases for the general behaviour). A basic implementation would look something like:

```
const char *S = nullptr;
for (const char *P = Str.begin(); P != Str.end(); ++P) {
  if (!isPrint(P)) {
    if (P - S >= 4)
      printStr(S, P);
    S = nullptr;
  }
}
```

This wouldn't print the trailing characters at all. A naive fix would add the following after the loop:
```
if (S != nullptr)
  printStr(S, Str.end());
```
When the check should actually be `if (S != nullptr && Str.end() - S >= 4)`. So this test case is checking that second clause in the final if.

Note that this and the original eof.test were written to fix a bug where that final if was missing.

================
Comment at: test/tools/llvm-strings/eof.test:1
+# Show that llvm-strings prints the last string in the input even if no
+# unprintable character follows it.
----------------
I'm not sure why my patch isn't showing this, but this has been renamed from "terminator.test".

================
Comment at: test/tools/llvm-strings/eof.test:5
 RUN: echo -n abcdefg | llvm-strings - | FileCheck %s
 CHECK: abcdefg
----------------
grimar wrote:
> What about combining this and `eof-no-string.test` test into one?
I could do that, but I kept them separate because that's how they were previously. If you'd like me to fold them together, I can do so.

================
Comment at: test/tools/llvm-strings/file-filename.test:10
+CHECK:      [[FILE]]: abcd
+CHECK-NEXT: [[FILE]]: hijk
----------------
grimar wrote:
> My emotion for this test at first was: "what about multiple inputs?". Then I found
> `multiple-inputs.test` test below. I would just inline it here.
Okay.

================
Comment at: test/tools/llvm-strings/help.test:12
+CATEG: General options:
+CATEG: Generic Options:
 CHECK: @FILE
----------------
grimar wrote:
> Note: I am not sure why "Options" is in upper case when "Generic", but lower case when "General",
> but my concern is that it makes the test case to be potentially brittle probably:
> 
> If you have `--implicit-check-not="Generic Options:"`, then if somebody deside to change "Option"->"option",
> then you'll get a broken test case. Not sure if we should think about this scenario in that way though.
I agree that the inconsistent case is bad. I'm not sure of a good alternative (apart from making it a regex pattern of [Oo]). I'll give it a bit more thought.

================
Comment at: test/tools/llvm-strings/length.test:13
+RUN: llvm-strings -n 4 %t | FileCheck --check-prefix CHECK-4 %s --implicit-check-not={{.}}
+RUN: llvm-strings -n 5 %t | FileCheck --check-prefix CHECK-5 %s --implicit-check-not={{.}}
+
----------------
grimar wrote:
> What if `-n 6` or `-n -1` or `-n foo`?
`-n -1` is actually broken. It looks like the command-line parser converts it to an unsigned, resulting in it being the equivalent of UINT_MAX. As GNU strings allows `-1` as an alias for `-n 1`, we probably don't want to try to address this immediately, and fix that issue first, then see if the problem is still possible afterwards.

I'll add `-n foo` and `-n 6` cases.

================
Comment at: test/tools/llvm-strings/version.test:1
+# Show that --version works for llvm-strings.
+
----------------
grimar wrote:
> I.d probably just put this into `help.test`. What do you think?
I prefer to keep each switch to a separate test, unless they're intricately linked (in this case --help and --version are not really linked).

================
Comment at: test/tools/llvm-strings/version.test:4
+RUN: llvm-strings --version | FileCheck %s
+CHECK: LLVM version
----------------
rupprecht wrote:
> This is actually testing the default cmake config; see this comment from `llvm/test/tools/llvm-readobj/basic.test`:
> 
> ```
> # Test version switch.
> RUN: llvm-readobj --version | FileCheck %s --check-prefix=VERSION
> RUN: llvm-readelf --version | FileCheck %s --check-prefix=VERSION
> # In default configuration we could match "LLVM version", but the "LLVM" part
> # can be changed with PACKAGE_NAME in CMake, so we match only version.
> VERSION: version
> ```
Thanks! I'll fix that.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66015/new/

https://reviews.llvm.org/D66015