[PATCH] D73574: Omit "Contents of" headers when -no-leading-headers is specified.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/X86/macho-cstring-dump.test:10-11
 NO_ADDR: Contents of (__TEXT,__cstring) section
 NO_ADDR: Hello world\n
 NO_ADDR-NOT: 000000000000003b
 
----------------
mtrent wrote:
> jhenderson wrote:
> > Not related to this patch, but I just spotted a bug here, which you might want to fix in a follow-up. This doesn't actually prove that the address is not printed, because it only shows that the address is not printed after "Hello world\n". I think just moving the line up a row would be enough, possibly in conjunction with --match-full-lines or similar.
> So all of these FileCheck scripts are fragile in one way or another.
> 
> Changing the second line to "NO_ADDR-NEXT" would address the specific issue you address. It doesn't address the problem of the address being printed head of the "Contents of" line, or if the address is printed using some other format. What's more, FileCheck is 'resilient' to some things, such as differing whitespace: this is both a really good thing and a really bad thing depending on what production tools expect. 
> 
> I'm happy to receive the feedback and to consider approaches in the future. Additional points of view polishes my understanding of the problem.
FileCheck's --implicit-check-not is useful for making sure some pattern doesn't appear anywhere in the output. That is often the way to go, though it has some downsides, especially with shorter strings (e.g. "bar" appearing in somebody's username, which in turn appears in a file path). You can also use --strict-whitespace and --match-full-lines if formatting is important for a given test case.

Regarding the NO_ADDR-NEXT suggestion, my personal view is that given there is a test case that shows the address is printed between the headers and the string when it is enabled, I think we only need to show that it is not printed at that location when the switch is specified.


================
Comment at: llvm/test/tools/llvm-objdump/X86/macho-cstring-dump.test:13-15
+NO_HEADERS-NOT: Inputs/hello.obj.macho-x86_64:
+NO_HEADERS-NOT: Contents of (__TEXT,__cstring) section
+NO_HEADERS: 000000000000003b	Hello world\n
----------------
jhenderson wrote:
> This is fine, but I could suggest a number of improvements to reduce duplication of the checks, by using multile check prefixes. No need to do that though, unless you want to.
More concrete suggestion (no need to adopt it though). Use something like the following patterns:

```
FILE: Inputs/hello.obj.macho-x86_64:
HEADER: Contents of (__TEXT,__cstring) section
ADDR: 000000000000003b
CHECK: {{^}}  Hello world\n
```

You then specify the combination of prefixes that correspond to the case, with --implicit-check-not to show the other strings don't appear. E.g. for the default case (i.e. print everything), use `--check-prefixes=FILE,HEADER,ADDR,CHECK`. The {{^}} ensures that the "Hello world" string matches the start of a line, or the end of the previous match (I think - not verified that this definitely will work), which means you'd want --match-full-lines to show the string appears on the same line as the address.

As to whether this is more readable or not is up for debate, but it does reduce the amount of checking.

It's also given me an idea for a new FileCheck feature. I'll post something on the mailing list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73574