libdevcore/JSON.h - new API by aarlt · Pull Request #3532 · argotorg/solidity

aarlt · 2018-02-15T22:44:13Z

removed deprecated api to be compatible with newer versions of jsoncpp
usage of strict-mode parsing
added json-specific tests in test/libdevcore/JSON.cpp
added unexpected_trailing_test to test/libsolidity/StandardCompiler.cpp

axic · 2018-02-15T23:41:28Z

libdevcore/JSON.h

+/// \param _json [out] resulting JSON object
+/// \param _errs [out] Formatted error messages
+/// \return \c true if the document was successfully parsed, \c false if an error occurred.
+inline bool parse(Json::CharReaderBuilder& _builder, const std::string &_input, Json::Value *_json, std::string *_errs)


Instead of pointers can we use references?

I also thought about this. The good thing with pointers are, that the direction of read/write is more visible - but thats mainly somehow just a matter of taste. Of course this would only make sense, if it's not only used in libdevcore/JSON.h.

Our general guideline is to only use a pointer if the pointer can be nullptr and references otherwise.

Ok. I will just change _json to be a reference.

axic · 2018-02-15T23:43:16Z

Most of the helpers were inline in JSON.h because they were tiny. The new version of the helpers seem to be a bit bigger, but more importantly they are used in more places than before. I think it makes sense moving the implementation out of the header.

I might be wrong about this, any opinions @chriseth @aarlt ?

axic · 2018-02-15T23:44:36Z

libsolc/libsolc.cpp

 string compileMulti(string const& _input, bool _optimize, CStyleReadFileCallback _readCallback = nullptr)
 {
-	Json::Reader reader;
+	std::string errs;


The std namespace is imported in this file.

Also it could be just errors to avoid abbreviations.

Sure! I really love explicitly written namespaces - but I will change my coding style for you. But errs should really be renamed to errors.

axic · 2018-02-15T23:44:51Z

libsolc/libsolc.cpp

+	std::string errs;
 	Json::Value input;
-	if (!reader.parse(_input, input, false))
+	if (!dev::jsonParseStrict(_input, &input, &errs))


The dev namespace is imported in this file.

axic · 2018-02-15T23:45:30Z

libsolidity/interface/StandardCompiler.cpp

 	Json::Value input;
-	Json::Reader reader;
-
+	std::string errs;


Same comments here as above.

axic · 2018-02-15T23:45:53Z

test/Metadata.cpp

 {
 	Json::Value metadata;
-	if (!Json::Reader().parse(_metadata, metadata, false))
+	if (!dev::jsonParseStrict(_metadata, &metadata))


This file is within the dev namespace.

axic · 2018-02-15T23:46:23Z

test/RPCSession.cpp

 	Json::Value config;
-	BOOST_REQUIRE(Json::Reader().parse(c_configString, config));
-	for (auto const& account: _accounts)
+	BOOST_REQUIRE(dev::jsonParseStrict(c_configString, &config));


Similarly, this file imports the dev namespace.

axic · 2018-02-15T23:47:36Z

test/libdevcore/JSON.cpp

+
+	// comments are allowed in strict-mode? - that's strange...
+	BOOST_CHECK(
+			dev::jsonParseStrict("{\"1\":3, // awesome comment\n\"2\":\"2\",\"3\":{\"3.1\":\"3.1\",\"3.2\":5}}", &json,


In these files please follow the coding style, roughly:

BOOST_CHECK(dev::jsonParseStrict( "{\"1\":3, // awesome comment\n\"2\":\"2\",\"3\":{\"3.1\":\"3.1\",\"3.2\":5}}", &errors ));

Yes, thats much better.

axic · 2018-02-15T23:48:11Z

test/libsolidity/SolidityNatspecJSON.cpp

-			"\n but got:\n" << generatedDocumentation.toStyledString()
-		);
+		dev::jsonParseStrict(_expectedDocumentationString, &expectedDocumentation);
+		BOOST_CHECK_MESSAGE(expectedDocumentation == generatedDocumentation,


Please do not change the coding style here.

axic · 2018-02-15T23:48:31Z

test/libsolidity/StandardCompiler.cpp

-	BOOST_CHECK(containsError(result, "JSONError", "Input is not a JSON object."));
-	BOOST_CHECK(!containsError(result, "JSONError", "* Line 1, Column 1\n  Syntax error: value, object or array expected.\n"));
+	BOOST_CHECK(containsError(result, "JSONError", "* Line 1, Column 1\n  A valid JSON document must be either an array or an object value.\n"));
+	BOOST_CHECK(!containsError(result, "JSONError","* Line 1, Column 1\n  Syntax error: value, object or array expected.\n"));


Please also avoid changing the coding style in the lines above.

aarlt · 2018-02-16T00:15:18Z

Most of the helpers were inline in JSON.h because they were tiny. The new version of the helpers seem to be a bit bigger, but more importantly they are used in more places than before. I think it makes sense moving the implementation out of the header.

I might be wrong about this, any opinions @chriseth @aarlt ?

It's definitely on the edge to get moved out of the header. I would like to move it.

chriseth · 2018-02-16T08:14:28Z

libdevcore/JSON.h

+{
+
+/// StreamWriterBuilder that can be constructed with specific settings
+class StreamWriterBuilder : public Json::StreamWriterBuilder {


Please don't put a space in front of : (except in x ? y : z).

chriseth · 2018-02-16T08:14:58Z

libdevcore/JSON.h

+/// StreamWriterBuilder that can be constructed with specific settings
+class StreamWriterBuilder : public Json::StreamWriterBuilder {
+public:
+	explicit StreamWriterBuilder(const std::map<std::string, std::string> &_settings)


Please put the const after the type

As far as I know const T& is just an alternative notation of T& const. I know that this notation is sometimes preferred, but why exactly?

chriseth · 2018-02-16T08:16:56Z

libdevcore/JSON.h

+/// \param _input JSON input string
+/// \param _builder StreamWriterBuilder that is used to create new Json::StreamWriter
+/// \return serialized json object
+inline std::string print(Json::Value const &_input, Json::StreamWriterBuilder &_builder) {


Please put { on its own line and move & to the left: Json::Value const& _input

chriseth · 2018-02-16T08:19:02Z

libdevcore/JSON.h

 inline std::string jsonPrettyPrint(Json::Value const& _input)
 {
-	return Json::StyledWriter().write(_input);
+	static std::map<std::string, std::string> settings{{"indentation", "  "}};


Do we need this local variable?

To what local variable you're referring to? The static settings object is used to configure the static StreamWriterBuilder.

chriseth · 2018-02-16T08:20:05Z

libdevcore/JSON.h

+};
+
+/// CharReaderBuilder with strict-mode settings
+class StrictModeCharReaderBuilder : public Json::CharReaderBuilder {


I think those classes are better suited in an anonymous namespace in the .cpp file - they are only used by the functions in this header, right?

Yep, currently it's just used by the functions in header. I thought that it could make sense to allow others to use those classes. But I think that will probably never used, because we can already parse and print JSON objects, that are the most important operations. I moved it to the .cpp file.

aarlt · 2018-02-17T18:47:24Z

@axic @chriseth Why is there that error? Is that related to my changes?

     Error: Could not connect to your Ethereum client. Please check that your Ethereum client:
    - is running
    - is accepting RPC connections (i.e., "--rpc" option is used in geth)
    - is accessible over the network
    - is properly configured in your Truffle configuration file (truffle.js)

chriseth · 2018-02-19T14:28:58Z

I'm running a small cleanup on the coding style and will force-push to your branch, so please do not overwrite later.

chriseth · 2018-02-19T14:45:00Z

@aarlt the error is unrelated to your change.

@axic can be merged from my side.

axic · 2018-02-20T10:26:08Z

libdevcore/JSON.h

+/// \param _json [out] resulting JSON object
+/// \param _errs [out] Formatted error messages
+/// \return \c true if the document was successfully parsed, \c false if an error occurred.
+bool jsonParse(std::string const& _input, Json::Value& _json, std::string* _errs = nullptr);


Every use cases uses strict mode - do we want to keep this non-strict helper?

Good question.. will we need to parse non-strict JSON in the future?

I don't think so, though it's small we can keep it.

axic · 2018-02-20T10:27:24Z

From the strict mode documentation:

   /** \brief A configuration that is strictly compatible with the JSON
   * specification.
   * - Comments are forbidden.
   * - Root object must be either an array or an object value.
   * - Assumes Value strings are encoded in UTF-8
   */
  static Features strictMode();

Perhaps it makes sense adding two more tests: comments and invalid UTF-8 strings.

aarlt · 2018-02-20T18:17:19Z

@axic I already included a test to check the comments and I was really surprised that they where allowed. Now I just took a look to the jsoncpp code - I really don't know why they write here Comments are forbidden. - if allowComments_ is false, they just skip CommentTokens.

void Reader::skipCommentTokens(Token& token) {
  if (features_.allowComments_) {
    do {
      readToken(token);
    } while (token.type_ == tokenComment);
  } else {
    readToken(token);
  }
}

It looks like that they just disable the collection of comments in there AST. So there will be no error generated. Probably allowComments is just a translation error or something like that, it should be ignoreComments.

Regarding Assumes Value strings are encoded in UTF-8 I think it just means that it assumes that the values are UTF-8 encoded, there is also no real check regarding that. The only thing I saw in the code was a check regarding UTF-8 escape sequences - but that is of course also done with non-strict parsing.

aarlt · 2018-02-20T18:18:57Z

@chriseth Wow, I didn't knew that a force-push to my branch is possible. Good to know.

axic · 2018-02-20T19:40:14Z

I really don't know why they write here Comments are forbidden. - if allowComments_ is false, they just skip CommentTokens.

We should submit a patch for "super strict", pedantic or "canonical json" mode :)

Regarding Assumes Value strings are encoded in UTF-8 I think it just means that it assumes that the values are UTF-8 encoded, there is also no real check regarding that. The only thing I saw in the code was a check regarding UTF-8 escape sequences - but that is of course also done with non-strict parsing.

Ok - I assumed the parser would fail on an invalid UTF-8 sequence. I'll add such a test case anyway so we'll trigger a failure once/if they implement it.

…N parsing

aarlt mentioned this pull request Feb 15, 2018

cmake/jsoncpp.cmake: update to jsoncpp v1.8.4 #3467

Merged

axic reviewed Feb 15, 2018

View reviewed changes

chriseth reviewed Feb 16, 2018

View reviewed changes

axic added the nextrelease label Feb 16, 2018

aarlt force-pushed the libdevcore_new_json_api branch from b21c97d to b75e3a1 Compare February 16, 2018 17:36

chriseth force-pushed the libdevcore_new_json_api branch from b75e3a1 to c3bc0c7 Compare February 19, 2018 14:44

axic reviewed Feb 20, 2018

View reviewed changes

axic force-pushed the libdevcore_new_json_api branch from c3bc0c7 to c4ed18b Compare February 20, 2018 20:56

Add new JSON API for better abstraction and for supporting strict JSO…

0f29ac4

…N parsing

axic force-pushed the libdevcore_new_json_api branch from c4ed18b to 0f29ac4 Compare February 20, 2018 20:58

axic approved these changes Feb 20, 2018

View reviewed changes

axic merged commit 70790d4 into argotorg:develop Feb 20, 2018

Conversation

aarlt commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axic Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarlt commented Feb 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarlt commented Feb 17, 2018

Uh oh!

chriseth commented Feb 19, 2018

Uh oh!

chriseth commented Feb 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

aarlt commented Feb 15, 2018 •

edited

Loading

axic Feb 15, 2018 •

edited

Loading

axic commented Feb 15, 2018 •

edited

Loading