Page MenuHomePhabricator

[Polly][JSONExporter] Replace bundled Jsoncpp with LLVM's JSON.h. NFC.
ClosedPublic

Authored by Meinersbur on Jul 27 2018, 6:05 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Meinersbur created this revision.Jul 27 2018, 6:05 PM
sammccall accepted this revision.Jul 28 2018, 2:58 AM

Usage of JSON.h looks pretty good, thanks for doing this migration!

Have commented in a few places where different patterns might be useful. A few of these would allow unexpected input to be processed more safely; from what I can see the existing code is no safer in these cases so they are potential improvements rather than anything blocking.

Accepting as this seems like a straightforward infrastructure change, but you may want to wait for a review from someone who knows Polly for any deeper suggestions.

lib/Exchange/JSONExporter.cpp
125 ↗(On Diff #157821)

This helper function is correct, but maybe feels slightly awkward.

Pulling out the "sizes" child as it's own variable might be clearer:

json::Array Sizes;
...
Sizes.push_back("*");
...
return json::Object{{"sizes", std::move(Sizes)}};

Up to you.

229 ↗(On Diff #157821)

get("context")->getAsString() -> getString("context")

As well as being terser, you'll get None rather than UB when the key is entirely missing.
Usually that's better (e.g. in this case you'll get an assertion rather than just a segfault)

283 ↗(On Diff #157821)

As above, probably clearer to get the Array* first with JScop.getArray("statements"), then check if it's null. (Indicating either missing key or wrong type, but I doubt you need to distinguish here)

(There's a few more places below where similar suggestions apply)

587 ↗(On Diff #157821)

This will crash if "name" is missing or has a different type.
I think the nicest way to write this is to compare to the value of Array.getString("name") without unwrapping (which is an Optional<StringRef>).

The LHS will get implicitly converted to Optional in this case (though you may have to explicitly convert it to StringRef first).

This revision is now accepted and ready to land.Jul 28 2018, 2:58 AM

Usage of JSON.h looks pretty good, thanks for doing this migration!

Thanks for the review!

"JSONExporter" is meant for supporting development, esp. regression tests. Input verification is incomplete, before and after this patch, and could trigger UB. A GSoC students added some verification last year, but still not for all cases. This patch is meant as a conversion, so does not add additional verifications.

grosser accepted this revision.Jul 28 2018, 10:41 PM

This is a straightforward, but very nice infrastructure change. Thank you both for pushing JSON support ahead in LLVM!

If this was not clear. I am very OK with this change.

Meinersbur marked 3 inline comments as done.
  • Remove appendToArray helper function
  • Replace some get("")->getAsXYZ by getXYZ("")
This revision was automatically updated to reflect the committed changes.
huihuiz added a subscriber: huihuiz.Aug 1 2018, 4:28 PM
if (!ParseResult) {
  ParseResult.takeError();
  errs() << "JSCoP file could not be parsed\n";
  return false;
}
json::Object &jscop = *ParseResult.get().getAsObject();

What are you doing with the result of takeError()? Isn't this causing warn_unused_result build failure?
How could you tell the difference between success and failure? If ParseResult is null value, how could you call takeError() from it?

if (!ParseResult) {
  ParseResult.takeError();
  errs() << "JSCoP file could not be parsed\n";
  return false;
}
json::Object &jscop = *ParseResult.get().getAsObject();

What are you doing with the result of takeError()?

It exact reason for failure is ignored. We know that an error occurred and a generic message is printed to the user.

Isn't this causing warn_unused_result build failure?

Fixed in r338659. Also printing the error message.
It is a warning, hence should not have resulted in a build failure.

How could you tell the difference between success and failure?

Parsing failed iff !ParseResult evaluates to true. (Expected<T>::operator bool())

If ParseResult is null value, how could you call takeError() from it?

ParseResult is not a pointer, but an object of type llvm::Expected<T>. It cannot be null.