[PATCH] D49950: [Polly][JSONExporter] Replace bundled Jsoncpp with LLVM's JSON.h. NFC.

[Sam McCall via Phabricator via llvm-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: lib/Exchange/JSONExporter.cpp:125
     if (!SAI->getDimensionSize(i)) {
-      Array["sizes"].append("*");
+      appendToArray(Array["sizes"], "*");
       i++;
----------------
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.


================
Comment at: lib/Exchange/JSONExporter.cpp:229
+  isl::set NewContext = isl::set{
+      S.getIslCtx().get(), JScop.get("context")->getAsString().getValue()};
 
----------------
`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)


================
Comment at: lib/Exchange/JSONExporter.cpp:283
 
-  Json::Value statements = JScop["statements"];
+  const json::Array &statements = *JScop.get("statements")->getAsArray();
 
----------------
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)


================
Comment at: lib/Exchange/JSONExporter.cpp:587
 
-  if (SAI->getName() != Array["name"].asCString())
+  if (SAI->getName() != Array.get("name")->getAsString().getValue())
     return false;
----------------
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).


Repository:
  rPLO Polly

https://reviews.llvm.org/D49950