JonasToth added inline comments.
================ Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:386 - bool BoolValue = Bool->getValue(); + const bool BoolValue = Bool->getValue(); ---------------- 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. ================ Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:745 + // matchers, but that is currently impossible. + // + const auto *If = dyn_cast<IfStmt>(SwitchCase->getSubStmt()); ---------------- LegalizeAdulthood wrote: > JonasToth wrote: > > redundant empty comment line > Meh, it's not redundant. It's there to aid readability of a big block of > text by visually separating it from the associated code. > > Why is this a problem? Its subjective, I wouldn't do it so I thought you might have overlooked it, if we prefer this its fine from my side. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56303/new/ https://reviews.llvm.org/D56303 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
