Do we actually need two separate options here? Which combination of these would
you use for LLVM code?
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:111
@@ -110,4 +110,3 @@
return (getText(Result, *BinOp->getLHS()) + " " + NegatedOperator +
- " " + getText(Result, *BinOp->getRHS()))
- .str();
+ " " + getText(Result, *BinOp->getRHS())).str();
}
----------------
The text on the left is clang-formatted. I prefer to keep code
clang-format-clean, so if you don't like the result, please file a bug against
clang-format rather than changing the formatting by hand (I personally don't
care in this case).
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:121
@@ +120,3 @@
+
+bool get(const OptionsView &Options, StringRef LocalName) {
+ return Options.get(LocalName, 0U) != 0U;
----------------
I don't think this function makes code shorter or easier to understand. I'd
remove it and use the implementation directly (maybe with implicit conversion
to bool to avoid the ` == 0U` part).
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:211
@@ -201,5 +210,3 @@
bool Value, StringRef Id) {
- Finder->addMatcher(ifStmt(isExpansionInMainFile(),
- hasThen(ReturnsBool(Value, ThenLiteralId)),
- hasElse(ReturnsBool(!Value))).bind(Id),
- this);
+ if (ChainedConditionalReturn) {
+ Finder->addMatcher(ifStmt(isExpansionInMainFile(),
----------------
How about:
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
ChainedConditionalReturn ? anything() :
unless(hasParent(ifStmt())),
hasThen(ReturnsBool(Value, ThenLiteralId)),
hasElse(ReturnsBool(!Value))).bind(Id),
this);
? (if this works, of course)
Same below.
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:41
@@ +40,3 @@
+///
+/// When a conditional boolean return appears at the end of a chain of `if`,
+/// `else if` statements, the conditional statement is left unchanged unless
----------------
I'd deduplicate some text here:
/// When a conditional boolean return (or assignment) appears at the end
/// of a chain of `if`, `else if` statements, the conditional statement is
left
/// unchanged unless the option `ChainedConditionalReturn`
/// (`ChainedConditionalAssignmentis for an assignment) specified as non-zero.
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:52
@@ +51,3 @@
+ SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context);
+ virtual ~SimplifyBooleanExprCheck() {}
+
----------------
Please remove the empty destructor, the top base class has a virtual destructor
(we need a check for this as well ;).
http://reviews.llvm.org/D8996
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
