In http://reviews.llvm.org/D8996#155356, @alexfh wrote:
> Do we actually need two separate options here? Which combination of these
> would you use for LLVM code?
Since this is a readability check and "beauty is in the eye of the beholder", I
didn't want to make decisions for the user. This extension of the check
embraces LLVM preferences by default based on feedback from the use of the
check on the clang/LLVM code. (In particular, these two issues came up in the
review of changes to clang/lib/Parse and clang/lib/AST.)
================
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();
}
----------------
alexfh wrote:
> 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).
This change was introduced by clang-format from the latest. I don't apply any
of my own personal taste in formatting anymore, but routinely run everything
through clang-format before posting for review.
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:121
@@ +120,3 @@
+
+bool get(const OptionsView &Options, StringRef LocalName) {
+ return Options.get(LocalName, 0U) != 0U;
----------------
alexfh wrote:
> 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).
I meant to bring this up on the mailing list. I got stuck for a while because
`bool b = false; Options.get(LocalName, b);` totally failed when `LocalName`
was configured to `true`. I tried to dig into it and figure out why,
apparently there is a member function template that is `enable_if`'ed for
integral types and because `bool` is an integral type, it was trying to turn
`true` into an integer and silently failing, leaving my boolean setting unset.
I'd rather have the setting just plain work for booleans because it's weird to
turn this yes/no option into 1/0 in the configuration. So I was trying to have
a local function that did "what I wanted" from OptionView until I could figure
out how to get OptionView to properly handle boolean options.
================
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(),
----------------
alexfh wrote:
> 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.
I thought about that, but I didn't try it out. I can go either way.
================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:52
@@ +51,3 @@
+ SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context);
+ virtual ~SimplifyBooleanExprCheck() {}
+
----------------
alexfh wrote:
> Please remove the empty destructor, the top base class has a virtual
> destructor (we need a check for this as well ;).
OK. Most code bases where I've worked had explicit preferences for virtual
d'tors to remind readers that the class participates in a virtual method
hierarchy.
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
