================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:20
@@ +19,3 @@
+
+StringRef getText(const MatchFinder::MatchResult &Result, SourceRange Range) {
+ return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > Is this function used only once? Maybe inline it then?
> I'm getting conflicting reviews here.
>
> One reviewer wants me not to write inline on small functions and you're
> asking me to write `inline`.
>
> I honestly don't care which, but please tell me which request I should follow
> :-).
We're talking about different ways to inline a function. I mean inlining on the
source code level: take the function body and use it in the single place that
invokes it.
Sorry for the ambiguity ;)
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:54
@@ +53,3 @@
+CXXBoolLiteralExpr const *getBoolLiteral(MatchFinder::MatchResult const
&Result,
+ const char *const Id) {
+ CXXBoolLiteralExpr const *Literal =
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > Please use `StringRef` instead of `const char*` wherever it makes sense.
> I'm happy to do that with some guidelines on what "makes sense". In this
> case it is character literals coming down from the place where you were
> saying you preferred `const char Id[] = ...`.
>
> What am I gaining by using `StringRef` here?
"Makes sense" here means roughly "everywhere where you pass a string, and not a
raw pointer to const char".
> What am I gaining by using StringRef here?
Peace of mind (in some sense): when you see a StringRef argument, you know that
there is no expensive copying or calculation of the length going on (except
where you create a StringRef from a char pointer) . Just use StringRef
everywhere where you need a constant string (or a fragment of a string), and
there's a buffer with sufficient lifetime (string literal, std::string)
somewhere.
You also get fewer code review comments to deal with ;)
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:57
@@ +56,3 @@
+ Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id);
+ return Literal &&
+ Result.SourceManager->isMacroBodyExpansion(
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > nit: It looks better with parentheses and makes operator precedence clear:
> >
> > return (Literal &&
> >
> > Result.SourceManager->isMacroBodyExpansion(Literal->getLocStart()))
> > ? nullptr
> > : Literal;
> >
> Talk to clang-format. I'm not using any of my own whitespace preferences,
> just ramming it through clang-format and taking what it gives me.
clang-format doesn't add or remove parentheses. But if you add parentheses here
yourself, you'll get the formatting as in the comment above.
So I ask you to add parentheses and re-run clang-format on this line.
Sorry again for not being clear.
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:66
@@ +65,3 @@
+SimpleReturnsBool(bool Value, char const *const Id = nullptr) {
+ return returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));
+}
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > How about putting this into the `ReturnsBool` function?
> >
> > auto SimpleReturnsBool =
> > returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));
> I went through a couple iterations on this where I had some auto lambdas and
> then went to named functions with explicit return types. For this one I
> don't think the auto is less readable.
Note, there's no lambda needed, and no function as well. You just need a
certain matcher to use in a more complex matcher. `auto SomeMatcher =
someMatcher(...);` is just the simplest way to write it. The function would
make sense if you called it with different arguments, e.g.:
return anyOf(
SimpleReturnsBool(Value, SomeId),
compoundStmt(statementCountIs(1), has(SimpleReturnsBool(Value,
SomeOtherId))));
Otherwise, it's just simpler and more readable without a separate function:
auto SimpleReturnsBool =
returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));
return anyOf(SimpleReturnsBool,
compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:304
@@ +303,3 @@
+ : "";
+ const auto Replacement = "return " + StringRef(Negated ? "!" : "") + "(" +
+ getText(Result, *If->getCond()) + ")" + Terminator;
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > And here the use of auto is simply dangerous: this creates a Twine object
> > that is only supposed to be used as a temporary (because if your
> > `getText()` returned `std::string`, the Twine object would point to a
> > deleted memory region right after the end of the expression). Please use
> > `std::string` instead.
> My head is spinning with all this farbling around between `std::string`,
> `StringRef` and `Twine`. Is the proper usage of these string classes
> documented somewhere so that I can refer to it while preparing a patch and
> spend less time churning the patch just to make these adjustments?
Some documentation is here:
http://llvm.org/docs/ProgrammersManual.html#passing-strings-the-stringref-and-twine-classes
You can also read class comments on StringRef and Twine.
What's important here, is that any `StringRef + something` that compiles yields
a Twine. And one dangerous case is `StringRef + temporary std::string`. This
creates a Twine pointing to a temporary `std::string`. The temporary string
gets deleted right at the end of the expression, and the Twine continues to
point to the deleted memory. So one should (almost) never store a Twine. The
correct way to use it is:
std::string Result = (llvm::Twine("...") + "..." +
someFunctionReturningAStdStringOrAStringRef() + ...).str();
http://reviews.llvm.org/D7648
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
