[PATCH] D67632: [libTooling] Introduce new library of source-code builders.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp:68
+std::string tooling::buildParens(const Expr &E, const ASTContext &Context) {
+  StringRef ExprText = getText(E, Context);
+  if (mayNeedParens(E))
----------------
ymandel wrote:
> gribozavr wrote:
> > `buildDereference` below checks for `getText` returning an empty string; this function does not. Why?
> Regarding this and the issue of returning `llvm::Optional`: the code overall is rather inconsistent in its handling of empty strings returned from `getText`. This is only one example.
> 
> sorry for this -- this code was mostly collected from various other places in the codebase and I should have looked it over more carefully.
> 
> With that said -- I think we need to decide whether we systematically assume that `getText` doesn't return an empty string, or systematically check for it and return llvm::None in that case.  We have the choice becuase there is no risk of UB from ignoring the empty text.   Any checks in the code seem only designed to propagate the "emptiness" for the caller's sake.
> 
> I'm inclined to state a precondition on the all of the functions that `getText(E)` is non empty, rather than sprinkle optionals throughout the API. My reasoning is that checking the result of these functions is "too late" in the process. A failure here is caused by passing a node that has no corresponding source code. If the caller is potentially dealing with such phantom nodes, they should know that before trying to construct code with them. Any check they can do on the result of `getText` they should have done earlier with a more appropriate predicate of the node.
> 
> That said, I understand the preference for defensiveness (don't assume the caller will get it right). I'm also fine with a compromise of asserting the non-emptiness of any internal call to `getText` although that might be almost as messy as just handling it.
> 
> WDYT?
It is true that there is no risk of UB (in terms of C++ spec), but there is a garbage-in/garbage-out issue. I don't think users will appreciate garbage out, because I don't think they have the ability to check the precondition properly -- after all, they don't know how exactly `getText` will be called.

Even if we figure out how to specify for users to check the precondition, they would always have to check it in order to use these APIs properly. After all, the source transformation tools that users write can't generally assume what code *their users* will throw at those tools. And if someone wants to throw together a simple tool prototype, they can always force-unwrap the optional, it is just an extra `*`.

Therefore, I believe embedding the check in these APIs will create a significantly better usage experience, and does not create much of a burden for users who don't want to handle errors.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67632/new/

https://reviews.llvm.org/D67632