[PATCH] D116787: [llvm-readobj][MachO] Add option to sort the symbol table before dumping (MachO only, for now).

Wed Mar 30 06:52:25 PDT 2022

oontvoo added inline comments.

================
Comment at: llvm/test/tools/llvm-readobj/sort-symbols.test:16
+# CHECK: Format: aixcoff-rs6000
+# CHECK-NOT: warning '{{.+}}_macho': --sort-symbols is not supported yet for this format.
+# CHECK: Format: Mach-O 64-bit x86-64
----------------
oontvoo wrote:
> oontvoo wrote:
> > oontvoo wrote:
> > > jhenderson wrote:
> > > > oontvoo wrote:
> > > > > jhenderson wrote:
> > > > > > I'd consider pruning this back to just `warning '{{.+}}_macho':` to reduce the risk of false negatives due to a slight change in the warning message.
> > > > > Wouldn't that make the test more prone to false positives? that is, if some new warning pops up somewhere else, this would trip. So i'm going to keep this.
> > > > Yes, it would, but we probably shouldn't be invoking behaviour that causes those warnings anyway, so they're harmless (it would be easy to fix the test if it did trigger one in the future).
> > > > 
> > > > The test as it was before the last edit demonstrates why overly strict CHECK-NOTs are not much use, because typos can cause them to pass spuriously.
> > > But that's not this test's job to guard against *OTHER* kinds of warnings.
> > Furthermore, false negatives/brittle tests are just as frustrating.
> > Yes, it would, but we probably shouldn't be invoking behaviour that causes those warnings anyway, so they're harmless (it would be easy to fix the test if it did trigger one in the future).
> 
> I disagree with the assertion that it's easy to fix these. Imagine there were a dozen tests similar to this one which were not expecting some  warnings, then someone added a  new warning and they would have to go update all these tests, even though it's not their fault. (it is the test's fault that it casts too wide a nest on the warning).
> 
s/nest/net

Do you have any other comment on this patch because it seems we've been back on forth for a very long time and it doesn't seem to get any more progress ...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787