Re: [Lldb-commits] [PATCH] D24369: [support] copying JSON parser/writer from lldb
aizatsky added a comment. 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. > 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 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 https://reviews.llvm.org/D24369 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24369: [support] copying JSON parser/writer from lldb
aizatsky updated this revision to Diff 70877.
aizatsky added a comment.
- getting rid of StringExtractor
- renaming methods to lowerCase.
https://reviews.llvm.org/D24369
Files:
.clang-tidy
include/llvm/Support/JSON.h
lib/Support/CMakeLists.txt
lib/Support/JSON.cpp
unittests/Support/CMakeLists.txt
unittests/Support/JSONTest.cpp
Index: unittests/Support/JSONTest.cpp
===
--- /dev/null
+++ unittests/Support/JSONTest.cpp
@@ -0,0 +1,46 @@
+//===- llvm/unittest/Support/JSONTest.cpp - JSON.cpp tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "llvm/Support/JSON.h"
+
+using namespace llvm;
+
+static ::testing::AssertionResult MatchRoundtrip(std::string Text) {
+ JSONParser Parser(Text.c_str());
+ auto Obj = Parser.parseJSONValue();
+
+ if (!Obj) {
+return Text == "null"
+ ? ::testing::AssertionSuccess()
+ : ::testing::AssertionFailure() << "can't parse input: " << Text;
+ }
+
+ std::string S;
+ raw_string_ostream Out(S);
+ Obj->write(Out);
+
+ std::string Actual = Out.str();
+ if (Actual != Text) {
+return ::testing::AssertionFailure() << "expected: " << Text
+ << " actual: " << Actual;
+ }
+ return ::testing::AssertionSuccess();
+}
+
+TEST(JSON, Roundtrip) {
+ EXPECT_TRUE(MatchRoundtrip("0"));
+ EXPECT_TRUE(MatchRoundtrip("3.145150e+00"));
+ EXPECT_TRUE(MatchRoundtrip("{}"));
+ EXPECT_TRUE(MatchRoundtrip("{\"a\":1,\"b\":2}"));
+ EXPECT_TRUE(MatchRoundtrip("[]"));
+ EXPECT_TRUE(MatchRoundtrip("[0]"));
+ EXPECT_TRUE(MatchRoundtrip("[1,\"two\",3]"));
+}
Index: unittests/Support/CMakeLists.txt
===
--- unittests/Support/CMakeLists.txt
+++ unittests/Support/CMakeLists.txt
@@ -19,6 +19,7 @@
ErrorTest.cpp
ErrorOrTest.cpp
FileOutputBufferTest.cpp
+ JSONTest.cpp
LEB128Test.cpp
LineIteratorTest.cpp
LockFileManagerTest.cpp
Index: lib/Support/JSON.cpp
===
--- /dev/null
+++ lib/Support/JSON.cpp
@@ -0,0 +1,595 @@
+//===- JSON.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "llvm/Support/JSON.h"
+
+#include
+#include
+
+#include "llvm/Support/ErrorHandling.h"
+
+using namespace llvm;
+
+std::string JSONString::jsonStringQuoteMetachars(const std::string &S) {
+ if (S.find('"') == std::string::npos)
+return S;
+
+ std::string Output;
+ const size_t Size = S.size();
+ const char *Chars = S.c_str();
+ for (size_t I = 0; I < Size; I++) {
+unsigned char Ch = *(Chars + I);
+if (Ch == '"') {
+ Output.push_back('\\');
+}
+Output.push_back(Ch);
+ }
+ return Output;
+}
+
+JSONString::JSONString() : JSONValue(JSONValue::Kind::String), Data() {}
+
+JSONString::JSONString(const char *S)
+: JSONValue(JSONValue::Kind::String), Data(S ? S : "") {}
+
+JSONString::JSONString(const std::string &S)
+: JSONValue(JSONValue::Kind::String), Data(S) {}
+
+void JSONString::write(raw_ostream &OS) const {
+ OS << "\"" << jsonStringQuoteMetachars(Data) << "\"";
+}
+
+uint64_t JSONNumber::getAsUnsigned() const {
+ switch (TheDataType) {
+ case DataType::Unsigned:
+return Data.Unsigned;
+ case DataType::Signed:
+return (uint64_t)Data.Signed;
+ case DataType::Double:
+return (uint64_t)Data.Double;
+ }
+ llvm_unreachable("Unhandled data type");
+}
+
+int64_t JSONNumber::getAsSigned() const {
+ switch (TheDataType) {
+ case DataType::Unsigned:
+return (int64_t)Data.Unsigned;
+ case DataType::Signed:
+return Data.Signed;
+ case DataType::Double:
+return (int64_t)Data.Double;
+ }
+ llvm_unreachable("Unhandled data type");
+}
+
+double JSONNumber::getAsDouble() const {
+ switch (TheDataType) {
+ case DataType::Unsigned:
+return (double)Data.Unsigned;
+ case DataType::Signed:
+return (double)Data.Signed;
+ case DataType::Double:
+return Data.Double;
+ }
+ llvm_unreachable("Unhandled data type");
+}
+
+void JSONNumber::write(raw_ostream &OS) const {
+ switch (TheDataType) {
+ case DataType::Unsigned:
+OS << Data.Unsigned;
+break;
+ case DataType::Signed:
+OS << Data.Signed;
+break;
+ case DataType::Double:
+OS << Data.Double;
+break;
+ }
+}
+
+JSONTrue::JSONTrue() : JSONValue(JSONValue::Kind::True) {}
+
+void JSONTrue::write(raw_ostream &OS) const { OS << "true"; }
+
+JSONFalse::J
Re: [Lldb-commits] [PATCH] D24369: [support] copying JSON parser/writer from lldb
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 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24369: [support] copying JSON parser/writer from lldb
aizatsky updated this revision to Diff 70879.
aizatsky added a comment.
remove .clang-tidy
https://reviews.llvm.org/D24369
Files:
include/llvm/Support/JSON.h
lib/Support/CMakeLists.txt
lib/Support/JSON.cpp
unittests/Support/CMakeLists.txt
unittests/Support/JSONTest.cpp
Index: unittests/Support/JSONTest.cpp
===
--- /dev/null
+++ unittests/Support/JSONTest.cpp
@@ -0,0 +1,46 @@
+//===- llvm/unittest/Support/JSONTest.cpp - JSON.cpp tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "llvm/Support/JSON.h"
+
+using namespace llvm;
+
+static ::testing::AssertionResult MatchRoundtrip(std::string Text) {
+ JSONParser Parser(Text.c_str());
+ auto Obj = Parser.parseJSONValue();
+
+ if (!Obj) {
+return Text == "null"
+ ? ::testing::AssertionSuccess()
+ : ::testing::AssertionFailure() << "can't parse input: " << Text;
+ }
+
+ std::string S;
+ raw_string_ostream Out(S);
+ Obj->write(Out);
+
+ std::string Actual = Out.str();
+ if (Actual != Text) {
+return ::testing::AssertionFailure() << "expected: " << Text
+ << " actual: " << Actual;
+ }
+ return ::testing::AssertionSuccess();
+}
+
+TEST(JSON, Roundtrip) {
+ EXPECT_TRUE(MatchRoundtrip("0"));
+ EXPECT_TRUE(MatchRoundtrip("3.145150e+00"));
+ EXPECT_TRUE(MatchRoundtrip("{}"));
+ EXPECT_TRUE(MatchRoundtrip("{\"a\":1,\"b\":2}"));
+ EXPECT_TRUE(MatchRoundtrip("[]"));
+ EXPECT_TRUE(MatchRoundtrip("[0]"));
+ EXPECT_TRUE(MatchRoundtrip("[1,\"two\",3]"));
+}
Index: unittests/Support/CMakeLists.txt
===
--- unittests/Support/CMakeLists.txt
+++ unittests/Support/CMakeLists.txt
@@ -19,6 +19,7 @@
ErrorTest.cpp
ErrorOrTest.cpp
FileOutputBufferTest.cpp
+ JSONTest.cpp
LEB128Test.cpp
LineIteratorTest.cpp
LockFileManagerTest.cpp
Index: lib/Support/JSON.cpp
===
--- /dev/null
+++ lib/Support/JSON.cpp
@@ -0,0 +1,595 @@
+//===- JSON.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "llvm/Support/JSON.h"
+
+#include
+#include
+
+#include "llvm/Support/ErrorHandling.h"
+
+using namespace llvm;
+
+std::string JSONString::jsonStringQuoteMetachars(const std::string &S) {
+ if (S.find('"') == std::string::npos)
+return S;
+
+ std::string Output;
+ const size_t Size = S.size();
+ const char *Chars = S.c_str();
+ for (size_t I = 0; I < Size; I++) {
+unsigned char Ch = *(Chars + I);
+if (Ch == '"') {
+ Output.push_back('\\');
+}
+Output.push_back(Ch);
+ }
+ return Output;
+}
+
+JSONString::JSONString() : JSONValue(JSONValue::Kind::String), Data() {}
+
+JSONString::JSONString(const char *S)
+: JSONValue(JSONValue::Kind::String), Data(S ? S : "") {}
+
+JSONString::JSONString(const std::string &S)
+: JSONValue(JSONValue::Kind::String), Data(S) {}
+
+void JSONString::write(raw_ostream &OS) const {
+ OS << "\"" << jsonStringQuoteMetachars(Data) << "\"";
+}
+
+uint64_t JSONNumber::getAsUnsigned() const {
+ switch (TheDataType) {
+ case DataType::Unsigned:
+return Data.Unsigned;
+ case DataType::Signed:
+return (uint64_t)Data.Signed;
+ case DataType::Double:
+return (uint64_t)Data.Double;
+ }
+ llvm_unreachable("Unhandled data type");
+}
+
+int64_t JSONNumber::getAsSigned() const {
+ switch (TheDataType) {
+ case DataType::Unsigned:
+return (int64_t)Data.Unsigned;
+ case DataType::Signed:
+return Data.Signed;
+ case DataType::Double:
+return (int64_t)Data.Double;
+ }
+ llvm_unreachable("Unhandled data type");
+}
+
+double JSONNumber::getAsDouble() const {
+ switch (TheDataType) {
+ case DataType::Unsigned:
+return (double)Data.Unsigned;
+ case DataType::Signed:
+return (double)Data.Signed;
+ case DataType::Double:
+return Data.Double;
+ }
+ llvm_unreachable("Unhandled data type");
+}
+
+void JSONNumber::write(raw_ostream &OS) const {
+ switch (TheDataType) {
+ case DataType::Unsigned:
+OS << Data.Unsigned;
+break;
+ case DataType::Signed:
+OS << Data.Signed;
+break;
+ case DataType::Double:
+OS << Data.Double;
+break;
+ }
+}
+
+JSONTrue::JSONTrue() : JSONValue(JSONValue::Kind::True) {}
+
+void JSONTrue::write(raw_ostream &OS) const { OS << "true"; }
+
+JSONFalse::JSONFalse() : JSONValue(JSONValue::Kind::False) {}
+
+void JSO
Re: [Lldb-commits] [PATCH] D24369: [support] copying JSON parser/writer from lldb
aizatsky updated this revision to Diff 70946.
aizatsky marked 10 inline comments as done.
aizatsky added a comment.
cleanup
https://reviews.llvm.org/D24369
Files:
include/llvm/Support/JSON.h
lib/Support/CMakeLists.txt
lib/Support/JSON.cpp
unittests/Support/CMakeLists.txt
unittests/Support/JSONTest.cpp
Index: unittests/Support/JSONTest.cpp
===
--- /dev/null
+++ unittests/Support/JSONTest.cpp
@@ -0,0 +1,46 @@
+//===- llvm/unittest/Support/JSONTest.cpp - JSON.cpp tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "llvm/Support/JSON.h"
+
+using namespace llvm;
+
+static ::testing::AssertionResult MatchRoundtrip(std::string Text) {
+ JSONParser Parser(Text.c_str());
+ auto Obj = Parser.parseJSONValue();
+
+ if (!Obj) {
+return Text == "null"
+ ? ::testing::AssertionSuccess()
+ : ::testing::AssertionFailure() << "can't parse input: " << Text;
+ }
+
+ std::string S;
+ raw_string_ostream Out(S);
+ Obj->write(Out);
+
+ std::string Actual = Out.str();
+ if (Actual != Text) {
+return ::testing::AssertionFailure() << "expected: " << Text
+ << " actual: " << Actual;
+ }
+ return ::testing::AssertionSuccess();
+}
+
+TEST(JSON, Roundtrip) {
+ EXPECT_TRUE(MatchRoundtrip("0"));
+ EXPECT_TRUE(MatchRoundtrip("3.145150e+00"));
+ EXPECT_TRUE(MatchRoundtrip("{}"));
+ EXPECT_TRUE(MatchRoundtrip("{\"a\":1,\"b\":2}"));
+ EXPECT_TRUE(MatchRoundtrip("[]"));
+ EXPECT_TRUE(MatchRoundtrip("[0]"));
+ EXPECT_TRUE(MatchRoundtrip("[1,\"two\",3]"));
+}
Index: unittests/Support/CMakeLists.txt
===
--- unittests/Support/CMakeLists.txt
+++ unittests/Support/CMakeLists.txt
@@ -19,6 +19,7 @@
ErrorTest.cpp
ErrorOrTest.cpp
FileOutputBufferTest.cpp
+ JSONTest.cpp
LEB128Test.cpp
LineIteratorTest.cpp
LockFileManagerTest.cpp
Index: lib/Support/JSON.cpp
===
--- /dev/null
+++ lib/Support/JSON.cpp
@@ -0,0 +1,537 @@
+//===- JSON.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "llvm/Support/JSON.h"
+
+#include
+#include
+
+#include "llvm/Support/ErrorHandling.h"
+
+using namespace llvm;
+
+static std::string quoteJsonString(StringRef S) {
+ if (S.find('"') == std::string::npos)
+return S;
+
+ std::string Output;
+ Output.reserve(S.size());
+ for (char Ch : S.bytes()) {
+if (Ch == '"')
+ Output.push_back('\\');
+Output.push_back(Ch);
+ }
+ return Output;
+}
+
+JSONString::JSONString(StringRef S)
+: JSONValue(JSONValue::Kind::String), Data(S) {}
+
+void JSONString::write(raw_ostream &OS) const {
+ OS << "\"" << quoteJsonString(Data) << "\"";
+}
+
+void JSONNumber::write(raw_ostream &OS) const {
+ switch (TheDataType) {
+ case DataType::Unsigned:
+OS << Data.Unsigned;
+break;
+ case DataType::Signed:
+OS << Data.Signed;
+break;
+ case DataType::Double:
+OS << Data.Double;
+break;
+ }
+}
+
+JSONTrue::JSONTrue() : JSONValue(JSONValue::Kind::True) {}
+
+void JSONTrue::write(raw_ostream &OS) const { OS << "true"; }
+
+JSONFalse::JSONFalse() : JSONValue(JSONValue::Kind::False) {}
+
+void JSONFalse::write(raw_ostream &OS) const { OS << "false"; }
+
+JSONNull::JSONNull() : JSONValue(JSONValue::Kind::Null) {}
+
+void JSONNull::write(raw_ostream &OS) const { OS << "null"; }
+
+JSONObject::JSONObject() : JSONValue(JSONValue::Kind::Object) {}
+
+void JSONObject::write(raw_ostream &OS) const {
+ bool First = true;
+ OS << '{';
+ for (const auto &KV : Elements) {
+if (First)
+ First = false;
+else
+ OS << ',';
+OS << "\"" << quoteJsonString(KV.first) << "\"";
+OS << ':';
+KV.second->write(OS);
+ }
+ OS << '}';
+}
+
+bool JSONObject::set(StringRef Key, std::unique_ptr Value) {
+ if (Key.empty() || nullptr == Value.get())
+return false;
+ Elements[Key] = std::move(Value);
+ return true;
+}
+
+bool JSONObject::get(StringRef Key, JSONValue const **ValuePtr) const {
+ auto Iter = Elements.find(Key), End = Elements.end();
+ if (Iter == End)
+return false;
+ *ValuePtr = Iter->second.get();
+ return true;
+}
+
+JSONArray::JSONArray() : JSONValue(JSONValue::Kind::Array) {}
+
+void JSONArray::write(raw_ostream &OS) const {
+ bool First = true;
+ OS << '[';
+ for (const auto &Element : Elements) {
+if (First
Re: [Lldb-commits] [PATCH] D24369: [support] copying JSON parser/writer from lldb
Oh, that's great! Thanks for pointing this out. I'll just upload the cleaned up version in case lldb/ folks would like it. I'll figure out how to use our YAML library. On Fri, Sep 9, 2016 at 6:48 PM Chandler Carruth wrote: > chandlerc added a comment. > > In https://reviews.llvm.org/D24369#538472, @zturner wrote: > > > In https://reviews.llvm.org/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. > > > https://reviews.llvm.org/D24369 > > > > -- Mike Sent from phone ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24369: [support] copying JSON parser/writer from lldb
aizatsky abandoned this revision. Comment at: include/llvm/Support/JSON.h:207 @@ +206,3 @@ +private: + typedef std::map Map; + typedef Map::iterator Iterator; zturner wrote: > `DenseMap` seems like a better choice here. DenseMap doesn't seem to be defined for std::string keys. Comment at: include/llvm/Support/JSON.h:238 @@ +237,3 @@ + + JSONValue::SP getObject(Index I); + zturner wrote: > How about providing an overloaded `operator[]` and providing `begin()` and > `end()` functions so that it can be used in a range based for syntax? done for operator[]. begin() and end() will be tricky, because underlying storage has std::unique_ptr<> Comment at: include/llvm/Support/JSON.h:282-283 @@ +281,4 @@ + + std::string Buffer; + size_t Index; +}; zturner wrote: > 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. `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. Comment at: lib/Support/JSON.cpp:53 @@ +52,3 @@ + case DataType::Signed: +return (uint64_t)Data.Signed; + case DataType::Double: zturner wrote: > Can you use C++ style casting operators here and in other similar functions? Removed all 3 them and added get method. https://reviews.llvm.org/D24369 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
