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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
