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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added a comment.

Thanks for the review!  Agreed on all points and then some -- next revision will have a bunch of cleanups. I think the only major issue is the return type. See below.



================
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))
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67632