In D114225#3186242, @thakis wrote:

The test fails on Windows: http://45.33.8.238/win/50629/step_11.txt

Please take a look and revert for now if it takes a while to fix.

Just as a heads up, Dec 10th is the last day of my internship so unfortunately after that I won't be able to work on this change as actively as before. If there is still feedback to address a member of my team may be able to pick this up from where I left off. And thank you for taking the time to review all of my code changes these past months!

Fix up file-summary test.

Add param comments.
Update pretty-print tests.

Update docs formatting.
Update file-summary and pretty print tests.

Fix DelimitedScopeCtor test.

Add verifyAll method to test fixture.
Allow JSONScopedPrinter to be constructed with DelimitedScope.
Add DelimitedScope and pretty-print unit tests.

Add W.flush() to verifyScopedPrinter() method.

Update docs.
Clean up tests.

Use JSONScopedPrinter ctor to initialize outer-most [].

Update test fixture to hold ScopedPrinter as member.

Update missing enum to enum class
Update test fixture to handle printing outer {} and handle nothing printed at top level.

Add pretty-print test.

Add JSON FileSummary tests.
Add pretty-print option to llvm-readelf.

Add missing comma in error message.

Update ScopeHistory SmallVector size to 8

Yes, they should be added, but I'd be inclined to say that they should be in a separate patch, allowing us to focus this on the ScopedPrinter class specifically.

I can add these in a separate patch then.

Update enum naming.
Add APSInt implementation and test.
Add classof and kindOf test.

Add APSInt test.

Add printList implementation for APSInt.

Update JSONScopedPrinter::printBinaryImpl to print initial offset rather than including a byte index in each byte entry.
Small comment change.

Renamed lambda and expected output variables

Remove unneeded explicit cast in printList(...).

Update error messaging.
Misc formatting.

Remove Character block from JSONScopedPrinter::printBinaryImpl(...)

Add JSON implementation for new printList(...) overloads.
Update comment wording.

Add more printList(...) tests.

Update printList for uint8_t and int8_t and remove ArrayRef from printListImpl(...)

Add printList overloads (to handle future JSONScopedPrinter number and boolean printing).

Update printEnum test

Add missing tests and update existing ones.

Address patch comments.

Updated printRawFlagsImpl to be overloaded printFlagsImpl method.
Updated formatting.

Misc formatting.

Add JSONScopedPrinter specific tests.
Update printBinaryImpl structure to include index.
Add hexNumberToInt method.
Revert JSONScopedPrinter::printListImpl to only print strings.
Add printRawFlagsImpl method to JSONScopedPrinter.

Clean up lambda naming.

Make printFlags and printObject virtual.

Update formatting.

Update --elf-output-style error messaging.
Add printFileSummary method to ObjDumper.
Misc formatting.

Add JSONScopedPrinter tests.
Update some ScopedPrinter methods.
Fix some formatting.

In D114224#3142126, @jhenderson wrote:

Cross-posting this comment, since it's now applicable to this patch specifically: I think we probably need to do a bit of both: each method should be tested in isolation. I think we should ensure we have tests for the history stack, where it's applicable only. Basically, test the existing possible code paths, plus maybe one or two more interesting cases that test interaction between functions, if it seems sensible.

Add printBinaryImpl implementation.
Remove duplicated logic for json scope logic.
Update printHex to print numbers.
Clean up formatting.

Add abstraction to object/array begin/end.
Misc formatting and function renaming.

I think JSONScopedPrinter (and possibly ScopedPrinter too, if it doesn't already have them) could do with gtest unit tests. It should be fairly straightforward to write them, and would help give confidence in the methods, especially as we may not use all of them up-front. The tests would also help with documenting what the code actually does, which will aid in future reviewing.

Will do! Do we want to test each method in isolation? or possibly in conjunction with each other to test things like the history stack? or possibly both?

We need to consider what will happen when dumping archives containing non-ELF objects. In such a case, it won't be possible to dump JSON output (for now) for them. Should we emit an error/warning in this case?

I think emitting a warning to the error stream seems reasonable in this case. Since it may still be desirable to output the information for remaining ELF objects inside those archives.

For the *Internal functions, I think *Impl tends to be a more common naming pattern I've seen (the non-impl versions do the common work, and then call the virtual impl versions, as you've done).

Thanks for the insight! Addressed in D114223.

In D111658#3136905, @jhenderson wrote:

Thanks for thinking about this, and sorry for not coming back sooner (I've been dealing with deadlines, but am now free for a bit, so hopefully will have a bit more time). I quite like the "history stack" idea of the two new approaches, and I think the solutions to the other cases are viable. Note however that your two new diffs appear to be identical? Either that or I'm missing where the problem 3 approach differs in the implementation.

Do you have a preference for an approach of the three?

In D111658#3121940, @Jaysonyan wrote:

I think that having JSONScopedPrinter as a subclass of ScopedPrinter would have more downsides than upsides without major refactoring to how we interact with ScopedPrinter in llvm-readobj. I believe the biggest driving force to having JSONScopedPrinter as a subclass to ScopedPrinter is with the hope that each implementation of ObjDumper can be agnostic to the type of ScopedPrinter it is using (json or regular). Although I don't believe that this is possible for a few reasons:

In D111658#3117710, @jhenderson wrote:

Sorry, I'm a little bit busy, and have a number of other reviews that are calling for my attention too. I haven't had a chance to give this too much further thought yet. One initial thought I had was that things like printLine might just be a no-op for JSON output, although I think it might be nicer if we could wrap these sorts of calls behind another interface suitable for generic llvm-readobj usage. Unfortunately, I don't have the time right now to take a look at all usages, to see if there's a way we could represent such functions in a cleaner manner. I'll try to come back to this later this week, or next week, if I can.

@jhenderson I was wondering if you had any more thoughts on the design on ScopedPrinter/ ScopedPrinterBase/JSONScopedPrinter? As I see it right now, it's not very feasible to treat ScopedPrinter and JSONScopedPrinter uniformly due to the usage of methods like startLine(). So a parent-child structure between ScopedPrinter and JSONScopedPrinter might not be suitable.

Update readelf and readobj docs wording
Misc formatting and renaming changes

4. Change JSONScopedPrinter to derive from ReadobjScopedPrinter and implement the additional functions (and move it into the llvm-readobj code).