[PATCH] D137088: [llvm-readobj] Standardize JSON output for `Other` field

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/llvm-vs-json-format.test:1
+# Check that the LLVM and JSON formats differ when their implementations diverge.
+
----------------
paulkirth wrote:
> jhenderson wrote:
> > Having looked at this again, rather than a separate test showing the differences, I think I'd prefer the test cases being added to existing tests that test LLVM and GNU output styles. For the st_other case, that would be https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-readobj/ELF/symbol-visibility.test (which is essentially the "main" st_other test) and potentially both https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-readobj/ELF/aarch64-symbols-stother.test and https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-readobj/ELF/mips-symbols-stother.test.
> > 
> > I was hoping we'd be able to share CHECK patterns, but of course, for JSON output, the keys are quoted, so that doesn't really work.
> I'm a bit concerned that the number of tests and test cases will start to rapidly grow as we progress through this stack, since there is basically zero JSON testing as it stands, but let's see how this goes.
> 
> I'd also like to keep this test since it documents how the two formats differ in a single place, and subsequent patches build on top of it.
It's just showing what the other test cases show though, for example, the aarch test you've modified clearly shows the behaviour difference between the two for st_other fields.

If you think highlighting the difference in behaviour between the two is worthwhile (I'm not convinced that it is), it probably should be in the documentation, rather than duplicating test coverage.

Regarding the rapid test case growth, that's rather inevitable, I'd have thought: you've got a new(ish) file format, that hasn't been rigorously tested before. It clearly needs to be better tested, so it's rather unsurprising. Keeping this file around will just exacerbate the test growth, since it will become one giant soup of test cases, be hard to maintain, and be somewhat hard to read, I suspect going forwards.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/mips-symbols-stother.test:27
 
+
+# MIPS-JSON:      "Value": "foo",
----------------
Nit: no need for extra blank line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137088