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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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>;
+
----------------
xazax.hun wrote:
> aaron.ballman wrote:
> > xazax.hun wrote:
> > > 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?
> > I usually figure that if it's less than two pointers in size, it's sufficiently small, but maybe others have a different opinion. This class is probably a bit large for using dense containers.
> > 
> > I do worry a little bit about the hoops we're jumping through to make the class orderable -- is there a reason to avoid an unordered container instead?
> I am not insisting on using ordered containers but note that since I have no idea how to get a deterministic and efficient hash value of a `RecordDecl` I would likely just include the address of the canonical decl. So the order of the elements in the container would be non-deterministic both ways. But the algorithm (other than the debug dumps which are fixable) will not depend on the order of the elements.
I guess I was more speaking to the point that, if this is going to be unordered anyway, why not just use unordered containers and not attempt to impose an ordering in the first place? If the nondeterminism is not user-observable, then I'm not certain it matters.


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:70
+  bool operator<(const LifetimeContractVariable &O) const {
+    if (Tag != O.Tag)
+      return Tag < O.Tag;
----------------
xazax.hun wrote:
> aaron.ballman wrote:
> > I think it would be easier to read if the pattern was: `if (Foo < Bar) return true;` as opposed to checking inequality before returning the comparison.
> I think this might not be equivalent. 
> 
> The code below will always early return if the `Tag` is not the same.
> ```
> if (Tag != O.Tag)
>   return Tag < O.Tag;
> ```
> 
> The code below will only early return if the condition is true.
> 
> ```
> if (Tag < O.Tag)
>   return true;
> ```
> 
> So I think the pattern above is an optimization. 
> 
Good point about ordering requirements, but this is still extremely hard to read. Can you use `std::tie()` to simplify the logic somewhat?


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:79
+      if (RD != O.RD)
+        return std::less<const RecordDecl *>()(RD, O.RD);
+
----------------
xazax.hun wrote:
> aaron.ballman wrote:
> > This will have non deterministic behavior between program executions -- are you sure that's what we want? Similar below for fields.
> Good point. As far as the analysis is concerned the end result should always be the same. I see potential problems in the tests where the contents of some ordered data structures using this ordering is dumped. I do not see any natural (and preformant) way to order RecordDecls. (I can order FieldDecls using their indices.) Are you ok with sorting the string representation before outputting them as strings? This should solve the potential non-deterministic behavior of tests. 
Would it make sense to order the records by their source location when the pointer values are not equal? Or ordering on the fully-qualified name of the record?


================
Comment at: clang/lib/Sema/SemaType.cpp:7727
 
+    // Move function type attribute to the declarator.
+    case ParsedAttr::AT_LifetimeContract:
----------------
xazax.hun wrote:
> aaron.ballman wrote:
> > This is not an okay thing to do for C++ attributes. They have a specific meaning as to what they appertain to and do not move around to better subjects like GNU-style attributes.
> Yeah, I know that. But this is problematic in the standard. See the contracts proposal which also kind of violates this rule by adding an exception. Basically, this is the poor man's version of emulating the contracts proposal.  In the long term we plan to piggyback on contracts rather than hacking the C++ attributes.
The contracts proposal was not adopted and I am opposed to adding this change for any standard attribute. We've done a lot of work to ensure that those attributes attach to the correct thing based on lexical placement and I don't want to start to undo that effort.


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

https://reviews.llvm.org/D72810