[PATCH] D111658: Add JSON output skeleton to llvm-readelf

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D111658#3139223 <https://reviews.llvm.org/D111658#3139223>, @Jaysonyan wrote:

> In D111658#3136905 <https://reviews.llvm.org/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?
>
> The interface for `ScopedPrinter` can broadly be split into 2 categories. The first is methods that creates scopes, the second is printing values inside those scopes. The first categories consists of `objectBegin()`, `arrayBegin()`, `objectBegin(attr)`, `arrayBegin(attr)` (and their respective end methods). The second category are methods such as `printString(Label, Value)` which print actual values, usually as a key-value pair. The goal for both the history
> stack and array output approach is that when printing key-value pairs its inside the context of an object and when printing single values (ex: arrays, object, number, string), its inside the context of arrays.
>
> The history stack approach utilizes saved context to conditionally output surrounding `{}` inside `objectBegin(attr)/arrayBegin(attr)` when inside the context of an array.
> The array output approach doesn't hold on to any context but will rather always output arrays and enclosing `{}` for both `objectBegin()/arrayBegin()` and for all key-value pairs it will always output enclosing `{}`. This ensures all calls to `ScopedPrinter` output single values and will always be in the context of an array.
>
> Personally I prefer the history stack approach as well, but recognize the duplicated work of keeping a history stack inside both `JSONScopedPrinter` and `json::OStream`. The benefit of the array output is that it doesn't need to do any of this work but it comes at the expense of a somewhat unintuitive output.

Thanks. I'm not too fussed by this duplication, if there's no practical way of avoiding it, and I agree that the output is more logical in this form. I'm going to start reviewing the "history stack" version now.


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

https://reviews.llvm.org/D111658