[Lldb-commits] [PATCH] D24369: [support] copying JSON parser/writer from lldb

[Mike Aizatsky via lldb-commits]

aizatsky added a comment.

In https://reviews.llvm.org/D24369#538472, @zturner wrote:

> In https://reviews.llvm.org/D24369#538422, @aizatsky wrote:
>
> > In https://reviews.llvm.org/D24369#538213, @zturner wrote:
> >
> > > Having a JSON parser in LLVM seems like a reasonable idea, but I'm afraid the JSON parser that currently exists in LLDB won't do.  As Pavel mentioned, the code has a definite LLDB feel to it, but more importantly it depends on StringExtractor which I think probably doesn't belong in LLVM.  If you want to go down this route, I would start by deleting the StringExtractor changes from this CL.  90% of the stuff StringExtractor is used for, like hex string to number, string to integer, etc are covered by existing LLVM classes or methods.
> >
> >
> > My rationale was that reusing parser from a child project is better than building anything new from scratch.
>
>
> Agree that we always want to reuse code wherever possible, but LLVM has stricter requirements on code style and class design.  The JSON stuff itself can probably be a good starting point since clang-tidy doesn't have one (see my comment later), but I don't think `StringExtractor` belongs in LLVM.  Looking at the code, JSON really only uses 1 or 2 methods from `StringExtractor anyway, so I don't think it's a big thing to overcome.
>
> > 
>
> > 
>
> > > I'm a bit surprised there's not already a json parser in LLVM.  Maybe there's a reason for it.  I think this CL needs a few more reviewers on the LLVM side, so I'm adding a few people.
>
> > 
>
> > 
>
> > I can't actually find json parser within clang-tidy. Only references that I see lead eventually to YAML parsing:
>
>
> I was thinking of `lib/Tooling/JSONCompilationDatabase`, but looking at it, it seems to be pretty specialized and doesn't support arbitrary JSON.


I got rid of StringExtractor and also renamed all methods to follow the guidelines.
Let me know which other style inconsistencies you see there.


https://reviews.llvm.org/D24369