aaron.ballman added a subscriber: LegalizeAdulthood.
aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:425
Finder->addMatcher(
- ifStmt(isExpansionInMainFile(),
hasCondition(cxxBoolLiteral(equals(Value)).bind(BooleanId)))
----------------
njames93 wrote:
> 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.
Drat, that matches my digging too. I think the current patch should leave this
requirement in place so we can get the bug fix in while we figure out what to
do with this bit (assuming the bug fix doesn't rely on this change somehow). If
you want to remove the main file requirement in another patch, I'd suggest
adding @LegalizeAdulthood as a reviewer to see if they remember why this
condition was in place and (I believe) wasn't tested.
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
