[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

Thu Sep 12 10:36:31 PDT 2019

Szelethus added a comment.

In D67420#1666578 <https://reviews.llvm.org/D67420#1666578>, @NoQ wrote:

> In D67420#1666141 <https://reviews.llvm.org/D67420#1666141>, @Szelethus wrote:
>
> > I do!
>
>
> Hmm, it sounds like you want to force both Clang frontend and Clang-Tidy to use the same set of flags to control these options (?) Much like `-analyzer-config`, these flags will have different "style" compared to other flags in the tool. Which is probably not too bad for hidden frontend flags that control the Analyzer because they're anyway set by a separate GUI checkbox, but the inconsistency with other flags would be super glaring in case of Clang-Tidy CLI. Do we really want to go in that direction? I'll be much more comfortable with letting each tool deal with its flags the way it prefers - this doesn't look like a good place for code reuse to me.

There are two things I wanted to touch on, but kinda failed to emphasize it. First is the topic of whether `-analyzer-config` flags are fitting for a feature no longer specific to the analyzer, and the second is similar in nature, but in the actual code -- it doesn't doesn't feel natural to me that `AnalyzerOptions` is required to construct this, while at the same time we're trying to make diagnostics construction independent of the analyzer. But I really wouldn't like to overengineer this just for the sake of it :^)

On the first, I'm kinda meh, even if we went for it, it would be a separate revision I guess, but the second has me concerned, unless I'm not following something right.

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

https://reviews.llvm.org/D67420