[support] copying JSON parser/writer from lldb

[aizatsky]

Summary

This CL contains a copy of JSON parser/writer that can be found
in the lldb subproject with only necessary changes:

• files now follow LLVM naming and formatting guidelines
• raw_ostream used instead of the special formatted stream
• very basic parse/print roundtrip test for JSON.

As soon as this CL lands I will prepare the one that removes JSON
parser from lldb/ and uses parent's.

[labath]

I am cc'ing a bunch of people to make sure we don't do this behind anyone's back. I don't know whether you discussed this with anyone on lldb side (it's certainly new to me), but I think it deserves a wider audience, as this is the first case (I can recall) of moving code from lldb back to llvm.

That said, I think it's a great idea. However, you should sync up with @zturner, as he has imminent plans to do a StringExtractor refactor (D24013) within lldb and you're making a copy of it here.

Some other high-level questions (not directly related to this change, but I'd like to know where you're going with this):

• despite the renaming, the code still has a distinct lldb feel to it. I presume this will be followed by a bunch of CL's to bring it more in line with the rest of llvm (?)
• you are copying StringExtractor to llvm as well. Will you switch lldb to use that copy as well, or is the plan to bypass it completely and use llvm parsing primitives for the job?
• were you able to get lldb test suite running? If not, I can help you get that working.
• mostly out of curiosity: what is the motivation for this? I guess you have a need for a JSON parser in llvm ?

cheers.

[zturner]

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.

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.

[aizatsky]

In 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.

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:

tool/ClangTidyMain.cpp
102:JSON array of objects:
138:Specifies a configuration in YAML/JSON format:

ClangTidyOptions.cpp
36:// Map std::pair<int, int> to a JSON array of size 2.

ClangTidyOptions.h
245:/// \brief Parses LineFilter from JSON and stores it to the \p Options.
249:/// \brief Parses configuration from JSON and returns \c ClangTidyOptions or an

[zturner]

In D24369#538422, @aizatsky wrote:

In 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.

[clayborg]

I like the idea of moving this down into LLVM. We should fully LLVM-ize the code on the first pass though and take the LLDB style out of it.

[aizatsky]

• getting rid of StringExtractor
• renaming methods to lowerCase.

[aizatsky]

In D24369#538472, @zturner wrote:

In D24369#538422, @aizatsky wrote:

In 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.

[aizatsky]

remove .clang-tidy

[zturner]

include/llvm/Support/JSON.h

28

I scanned the entire patch and I'm not sure I understand the need for the covariant typedefs. The only type that ever seems to be used is JSONValue, so it seems to me like the other types are unnecessary.

Also, I'm not sure shared_ptr is necessary. Wouldn't unique_ptr work just fine? Then you could vend references to values instead of pointers.

48

I would make this a StringRef. Currently if someone calls this with a StringRef, it will make a copy. If you change the constructor to take a StringRef, then it won't take a copy regardless of whether it's called with a StringRef or a std::string.

57

I would return a StringRef here. Otherwise you're guaranteed to make a copy, if you return a StringRef it will only make a copy when you assign it to a std::string.

64

This could be a file static in the .cpp file, and also it could take a StringRef instead of a const std::string &.

203

Couple things here.

1. Key could be a StringRef
2. This method should probably be const.
3. You could make the underlying value a unique_ptr instead of a shared_ptr and return a reference. You could also have a tryGetValue in cases where you aren't sure if the value exists. Not sure what would be better.

208

DenseMap seems like a better choice here.

230–232

I don't know if the typedefs here help or hurt readability. I would just make them size_t, and remove the Iterator typedef as it doesn't seem to be used.

239

How about providing an overloaded operator[] and providing begin() and end() functions so that it can be used in a range based for syntax?

283–284

Instead of having a std::string here, which will copy the input JSON, I would make this a StringRef. Then you can delete the Index field as well, because the internal StringRef itself could always point to the beginning of the next char by using a Buffer = Buffer.drop_front(N) like pattern. This won't work if you ever have to do back-referencing across functions, but I don't think that is needed. Also if Buffer is a StringRef, a lot of the methods like peekChar() and skipSpaces become trivial one-liners that you wouldn't even need to provide a separate function for.

lib/Support/JSON.cpp

20

Input parameter should be a StringRef.

24

Should probably do an Output.reserve(S.size());

25–27

for (char CH : S)

54

Can you use C++ style casting operators here and in other similar functions?

116–117

for (const auto &I : Elements) {

188

This function should be const.

284

Don't like the idea of reimplementing string -> number parsing, but I'm not sure if LLVM has something to solve this for us.

[chandlerc]

In D24369#538472, @zturner wrote:

In D24369#538422, @aizatsky wrote:

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.

There is already a JSON parser in LLVM.

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.

It uses the YAML parser because JSON is a strict subset of YAML: https://en.wikipedia.org/wiki/JSON#YAML

Please don't add a JSON parser to LLVM. I would much rather teach LLDB to use the YAML parser and improve that parser if and where useful.

[aizatsky]

cleanup

[aizatsky]

include/llvm/Support/JSON.h

208

DenseMap doesn't seem to be defined for std::string keys.

239

done for operator[].

begin() and end() will be tricky, because underlying storage has std::unique_ptr<>

283–284

std::string => StringRef - done.

As for removing Index - I'm not sure. It is currently used in error reporting and is the only feedback we give when we encounter an error.

lib/Support/JSON.cpp

54

Removed all 3 them and added get<T> method.