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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang/include/clang/Tooling/Refactoring/SourceCodeBuilders.h:9
+///
+/// /file
+/// This file collects facilities for generating source-code strings.
----------------
"\file"


================
Comment at: clang/include/clang/Tooling/Refactoring/SourceCodeBuilders.h:10
+/// /file
+/// This file collects facilities for generating source-code strings.
+///
----------------
"source code"


================
Comment at: clang/include/clang/Tooling/Refactoring/SourceCodeBuilders.h:30
+
+/// Determines whether printing this expression in *any* expression requires a
+/// parentheses to preserve its meaning. This analyses is necessarily
----------------
s/a//

or s/a parentheses/a pair of parentheses/


================
Comment at: clang/include/clang/Tooling/Refactoring/SourceCodeBuilders.h:33
+/// conservative because it lacks information about the target context.
+bool mayNeedParens(const Expr &E);
+
----------------
`mayEverNeedParens`? (to emphasize conservativeness and lack of context)


================
Comment at: clang/include/clang/Tooling/Refactoring/SourceCodeBuilders.h:56
+/// Builds idiomatic source for the dereferencing of `E`: prefix with `*` but
+/// simplify when it already begins with `&`.  \returns empty string on failure.
+std::string buildDereference(const Expr &E, const ASTContext &Context);
----------------
Given that empty string is returned in a vanishingly small number of cases, it would be worth it returning `llvm::Optional<std::string>` to force the caller to handle the failure.


================
Comment at: clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp:77
+
+  if (Text.empty()) return std::string();
+  // Add leading '*'.
----------------
ymandel wrote:
> Eugene.Zelenko wrote:
> > Could return {}. Same in other places. By the word, did you run Clang-format over code?
> No, thanks for pointing that out.
> 
> I find `return std::string()` more readable, because it doesn't require that I check/know the return type of the function. But, if `return {}` is standard, I'm fine changing it. What's most common in the clang source?
I personally find `return "";` more readable. After all, we don't care about the specific type, we want to express that we return an empty string. The most natural way to write an empty string is a string literal `""`.


================
Comment at: clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp:21
+// Ignores implicit object-construction expressions in addition to the normal
+// implicit expressions that are ignored.
+const Expr *tooling::reallyIgnoreImplicit(const Expr &E) {
----------------
No need to repeat the comment from the header.


================
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))
----------------
`buildDereference` below checks for `getText` returning an empty string; this function does not. Why?


================
Comment at: clang/unittests/Tooling/SourceCodeBuildersTest.cpp:23
+
+// Create a valid translation-unit from a statement.
+static std::string wrapSnippet(StringRef StatementCode) {
----------------
"translation unit"


================
Comment at: clang/unittests/Tooling/SourceCodeBuildersTest.cpp:76
+  testPredicate(needParensAfterUnaryOperator, "3 + 5;", true);
+  testPredicate(needParensAfterUnaryOperator, "true ? 3 : 5;", true);
+
----------------
Also need tests for:
- overloaded binary operator `S(10) + S(20)`
- implicit conversions `void takeS(S); takeS(10 + 20);`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67632