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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
