[PATCH] D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions

Richard via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 20 12:42:22 PDT 2019


LegalizeAdulthood marked 4 inline comments as done.
LegalizeAdulthood added inline comments.
Herald added a subscriber: jdoerfert.


================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:386
 
-  bool BoolValue = Bool->getValue();
+  const bool BoolValue = Bool->getValue();
 
----------------
JonasToth wrote:
> aaron.ballman wrote:
> > LegalizeAdulthood wrote:
> > > JonasToth wrote:
> > > > `const` on values is uncommon in clang-tidy code. Please keep that consistent.
> > > I can change the code, but I don't understand the push back.
> > > 
> > > "That's the way it's done elsewhere" just doesn't seem like good reasoning.
> > > 
> > > I write const on values to signify that they are computed once and only once.  What is gained by removing that statement of once-and-only-once?
> > > "That's the way it's done elsewhere" just doesn't seem like good reasoning.
> > 
> > Keeping the code consistent with the vast majority of the remainder of the project is valid reasoning. I am echoing the request to drop the top-level const. We don't use this pattern for non-pointer/reference types and there's not a whole lot gained by using it inconsistently.
> Plus we do have a clang-tidy check (in the pipeline) that could do that automatically if the LLVM projects decides to go that route. So we won't suffer in the future, if we add the const.
You haven't addressed my point, which is that `const` is for values that don't change.  Instead, you're just repeating "we haven't done it that way" instead of refuting the utility of `const`.


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

https://reviews.llvm.org/D56303





More information about the cfe-commits mailing list