[PATCH] D72810: [LifetimeAnalysis] Add support for lifetime annotations on functions

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 14 inline comments as done.
xazax.hun added inline comments.


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:163
+// Maps each annotated entity to a lifetime set.
+using LifetimeContracts = std::map<LifetimeContractVariable, LifetimeSet>;
+
----------------
gribozavr2 wrote:
> Generally, DenseMap and DenseSet are faster. Any reason to not use them instead?
No specific reason other than I am always insecure about the choice. Dense data structures are great for small objects and I do not know where the break even point is and never really did any benchmarks. Is there some guidelines somewhere for what object size should we prefer one over the other?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3342
+  InGroup<LifetimeAnalysis>, DefaultIgnore;
+def warn_dump_lifetime_contracts : Warning<"%0">, InGroup<LifetimeDumpContracts>, DefaultIgnore;
+
----------------
gribozavr2 wrote:
> xazax.hun wrote:
> > gribozavr2 wrote:
> > > I think a warning that is a debugging aid is new territory.
> > Do you think a cc1 flag would be sufficient or do you have other ideas/preferences?
> Yes, I think that would be quite standard (similar to dumping the AST, dumping the CFG etc.)
I started to look into a cc1 flag, but it looks like it needs a bit more plumbing I expected as some does not have access to the CompilerInvocation object or the FrontendOptions. While it is not too hard to pass an additional boolean to Sema I also started to think about the user/developer experience (ok, regular users are not expected to do debugging). The advantage of warnings are that we get a source location for free, so it is really easy to see where the message is originated from. And while it is true that I cannot think of any examples of using warnings for debugging the Clang Static Analyzer is full of checks that are only dumping debug data as warnings.


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

https://reviews.llvm.org/D72810