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