Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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. The LHS will get implicitly converted to Optional in this case (though you may have to explicitly convert it to StringRef first). |
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.
This is a straightforward, but very nice infrastructure change. Thank you both for pushing JSON support ahead in LLVM!
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?
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.