njames93 marked an inline comment as done.
njames93 added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:425
Finder->addMatcher(
- ifStmt(isExpansionInMainFile(),
hasCondition(cxxBoolLiteral(equals(Value)).bind(BooleanId)))
----------------
aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > This is changing the behavior so that now it will diagnose in header
> > > files, no? Why is the correct change to replace this with
> > > `unless(isInTemplateInstantiation())` instead of adding the new matcher?
> > It's changing behaviour that arguably shouldn't have been in the first
> > place. But perhaps that change should go on a new patch or update the
> > description of this one
> I'll admit that the original code seems a bit suspect to me. I sort of wonder
> if it was being used to suppress diagnosing macros unless they're considered
> to be under the user's control. e.g., macros in headers may not be plausible
> to change but macros in source files are.
>
> If changes should be made here, I don't have strong opinions on whether it
> requires a separate patch or can be done in this one, but I'd like to better
> understand why the original code was incorrect (if it is in fact incorrect).
So having a look in the archives shows this condition was on the [[
https://reviews.llvm.org/D7648?id=19976 | first draft for this check. ]] But I
couldn't see any discussion about it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81336/new/
https://reviews.llvm.org/D81336
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
