[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

Wed Jan 1 09:56:36 PST 2020

aaron.ballman added a comment.

In D71846#1800480 <https://reviews.llvm.org/D71846#1800480>, @njames93 wrote:

> In D71846#1800411 <https://reviews.llvm.org/D71846#1800411>, @aaron.ballman wrote:
>
> > In D71846#1800401 <https://reviews.llvm.org/D71846#1800401>, @njames93 wrote:
> >
> > > In D71846#1800381 <https://reviews.llvm.org/D71846#1800381>, @aaron.ballman wrote:
> > >
> > > > In D71846#1800344 <https://reviews.llvm.org/D71846#1800344>, @njames93 wrote:
> > > >
> > > > > I'm in two minds about issuing a warning when scope restrictions prevent a fix. Do you think creating an option to enable or disable emitting warnings for cases where the scope prevents a fix would be a good idea?
> > > >
> > > >
> > > > It's not uncommon for fixits to only be generated under specific circumstances, so I'm wondering what your concern is with warning when we can't provide a fixit? The cases I am thinking about all seem reasonable to diagnose (are true positives) without fixing, but maybe you have different circumstances in mind.
> > >
> > >
> > > Right now an issue is raised for every else after return flag, but not all else after return flags can be fixed due to declaration statements and scope issues. My suggestion is that you can choose to warn about those cases or not. For example a developer may want else after return for when they need to limit the scope and getting a warning for it may be undesirable.
> >
> >
> > Okay, I can see the value in having an option for that -- especially given that silencing the diagnostic would add `// NOLINT` noise to the code. Do you think the option should default to diagnosing all cases, even ones without a fixit?
>
>
> Definitely default to diagnosing everything, that's how my current implementation looks right now.

SGTM

> Also is there a way to do options as bool because I don't particularly like putting 1 for true in the config file

Not to my knowledge.

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

https://reviews.llvm.org/D71846