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

This revision was automatically updated to reflect the committed changes. Closed by commit rGfd4b3f123d6e: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions. (authored by dergachev.a). Herald added a subscriber: steakhal. Repository: rG LLVM Github Monorepo CHANGES

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

NoQ updated this revision to Diff 277861. NoQ added a comment. Remove the `ShouldDisplayPathNotes` option as it never was an `AnalyzerOption` to begin with and it only makes things worse. F12338321: 488ny6.jpg CHANGES SINCE LAST ACTION

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

Szelethus accepted this revision. Szelethus marked 2 inline comments as done. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! Very well done! In D67420#2149461 , @vsavchenko wrote: > I strongly believe that decoupling

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

Szelethus added a comment. I've seen you resurrecting the thread, I'll just take a bit of time to remember what happened in the previous episodes! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67420/new/ https://reviews.llvm.org/D67420

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

vsavchenko added a comment. I do like this change even apart from the `clang-tidy` integration epic. I strongly believe that decoupling (when done right) makes things easier and this change seems like a good first step. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67420/new/

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

NoQ added a comment. @vsavchenko and others just joining: The backstory and the overall plan are in this thread: http://lists.llvm.org/pipermail/cfe-dev/2019-August/063092.html. Continued at http://lists.llvm.org/pipermail/cfe-dev/2019-September/063229.html. CHANGES SINCE LAST ACTION

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

NoQ added a reviewer: vsavchenko. NoQ added inline comments. Comment at: clang/include/clang/Analysis/PathDiagnostic.h:81 + /// because deterministic mode is always superior when done right, but + /// for some consumers is experimental and needs to be off by default. + bool

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

NoQ updated this revision to Diff 277652. NoQ marked 2 inline comments as done. NoQ added a comment. Herald added subscribers: ASDenysPetrov, martong. Rebase!! This work was on hiatus but i plan to continue. Address a review comment. There are two new path diagnostic options since the last

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

Szelethus added inline comments. Comment at: clang/include/clang/Analysis/PathDiagnostic.h:81 + /// because deterministic mode is always superior when done right, but + /// for some consumers is experimental and needs to be off by default. + bool

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

Szelethus added a comment. Aha, so every user gets to create their own `PathDiagnosticConsumerOptions` object, makes sense! There is no interface misconception, because `-analyzer-config` will only configure what the analyzer would tinket with. I like this patch! If you dont mind, I'd prefer

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

Szelethus added a comment. In D67420#1668474 , @NoQ wrote: > It's not that `AnalyzerOptions` is //necessary// to construct this, it's more > like //sufficient//. *sudden enlightenment* Ah, okay, right, I'm a dummie, I think I got what's happening

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

NoQ added a comment. In D67420#1668099 , @Szelethus wrote: > ...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

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

Szelethus added a comment. In D67420#1666578 , @NoQ wrote: > In 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

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

gribozavr added a comment. `PathDiagnosticConsumerOptions` is pretty lightweight, and is not passed around in hot paths if I understand correctly. What do you think about passing it by value everywhere? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67420/new/

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

NoQ updated this revision to Diff 219826. NoQ added a comment. Clean up a tiny bit of dead code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67420/new/ https://reviews.llvm.org/D67420 Files: clang/include/clang/Analysis/PathDiagnostic.h

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

NoQ updated this revision to Diff 219821. NoQ added a comment. Unforget to do the same for the text consumer. As a side effect, the factory function for the text consumer is no longer special, which will be less confusing when put in libAnalysis. Address minor comments, don't address major

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

NoQ added a comment. In D67420#1666050 , @gribozavr wrote: > Are you planning to move users of these options -- like `PlistDiagnostics` -- > to libAnalysis? Yup, i was supposed to do that in D67422 :) Repository:

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

NoQ added a comment. In D67420#1666141 , @Szelethus wrote: > - For these select 4 options that suffer compatibility issues, manually check > `AnalyzerOptions::ConfigTable`. *3 options. `ToolInvocation` is not really an option that you're ever supposed

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

NoQ added a comment. In 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

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

Szelethus added a comment. > @Szelethus: I could have stored `PathDiagnosticConsumerOptions` in > `AnalyzerOptions` by value and pass a const reference around, but it wasn't > pleasant to integrate with `AnalyzerOptions.def`. I.e., i'd have to implement > a new kind of option that doesn't

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

gribozavr added a comment. Are you planning to move users of these options -- like `PlistDiagnostics` -- to libAnalysis? Comment at: clang/include/clang/Analysis/PathDiagnostic.h:81 + /// because deterministic mode is always superior when done right, but + /// for some

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

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, baloghadamsoftware, Charusso. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet. Herald added a project: clang. NoQ added reviewers: alexfh,