[PATCH] D53483: [analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:146-149
+/// If you'd like to add a new -cc1 flag, add it to
+/// include/clang/Driver/CC1Options.td, add a new field to store the value of
+/// that flag in this class, and initialize it in
+/// lib/Frontend/CompilerInvocation.cpp.
----------------
NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > Szelethus wrote:
> > > > > > NoQ wrote:
> > > > > > > Nono, don't add more -cc1 flags :)
> > > > > > Code review is there to stop adding unnecessary -cc1 flags. Are we sure we wouldn't even like to document it? I myself will add at least 2 more -cc1 flags in the future (-analyzer-config-help, -analyzer-checker-option-help), that undoubtedly belong there.
> > > > > But these flags wouldn't define new analyzer options(?)
> > > > That is correct, but we do store similar cc1 flags here, because they belong to the the analyzer.
> > > > 
> > > > Although, hm, some of those would be more fitting as -analyzer-config flags, but I don't see how I could pull that off in a backward compatible way.
> > > Those were there before `AnalyzerOptions` were invented. That said, @george.karpenkov just made an experiment with converting `-analyzer-eagerly-assume` into `-analyzer-config eagerly-assume` (D51251) and it seems to have went well. For feature flags that were not really ever supposed to have been appended by GUIs, it is enough to just make sure that the correct behavior is enabled by default; there's no need to maintain backwards compatibility.
> > Hmm, maybe I could add that to my TODO list :)
> > 
> > I think the comment should still mention cc1 flags, but with an addition:
> > > Add a -cc1 only if your flag is related to the Static Analyzer, but doesn't actually change the actual analysis.
> Sounds good! Or something like that: "TODO: Some of these options are controlled by raw frontend flags. These should be eventually converted into -analyzer-config flags. New analyzer options should not be implemented as frontend flags. Frontend flags still make sense for things that do not affect the actual analysis."
Will do that! Aaaafter I'm confident enough that the previous patches cause no more trouble.


https://reviews.llvm.org/D53483