aaron.ballman added a comment. Thank you for the work on this!
================ Comment at: clang/include/clang/AST/LifetimeAttr.h:24 +/// An abstract memory location that participates in defining a lifetime +/// contract. A lifetime contract constrains lifetime of a +/// LifetimeContractVariable to be at least as big as the lifetime of other ---------------- constrains lifetime -> constrains the lifetime ================ Comment at: clang/include/clang/AST/LifetimeAttr.h:25 +/// contract. A lifetime contract constrains lifetime of a +/// LifetimeContractVariable to be at least as big as the lifetime of other +/// LifetimeContractVariables. ---------------- big -> long ================ Comment at: clang/include/clang/AST/LifetimeAttr.h:29 +/// The memory locations that we can describe are: return values of a function, +/// this pointer, any function parameter, a "drilldown" expression based on +/// function parameters, null etc. ---------------- What is a drilldown expression? That's not a term I'm familiar with. ================ Comment at: clang/include/clang/AST/LifetimeAttr.h:70 + bool operator<(const LifetimeContractVariable &O) const { + if (Tag != O.Tag) + return Tag < O.Tag; ---------------- 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. ================ Comment at: clang/include/clang/AST/LifetimeAttr.h:79 + if (RD != O.RD) + return std::less<const RecordDecl *>()(RD, O.RD); + ---------------- This will have non deterministic behavior between program executions -- are you sure that's what we want? Similar below for fields. ================ Comment at: clang/include/clang/AST/LifetimeAttr.h:153-154 + + LifetimeContractVariable(TagType T) : Tag(T) {} + LifetimeContractVariable(const RecordDecl *RD) : RD(RD), Tag(This) {} + LifetimeContractVariable(const ParmVarDecl *PVD, int Deref) ---------------- These constructors should probably be `explicit`? ================ 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: > 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? ================ Comment at: clang/include/clang/Basic/Attr.td:2902 +def LifetimeContract : InheritableAttr { + let Spellings = [CXX11<"gsl", "pre">, CXX11<"gsl", "post">]; ---------------- I think the attribute should probably only be supported in C++ for now, and should have `let LangOpts = [CPlusPlus];` in the declaration. WDYT? ================ Comment at: clang/include/clang/Basic/Attr.td:2941 + }]; + let Documentation = [Undocumented]; // FIXME +} ---------------- I agree with the FIXME comment. ;-) ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3342 + InGroup<LifetimeAnalysis>, DefaultIgnore; +def warn_dump_lifetime_contracts : Warning<"%0">, InGroup<LifetimeDumpContracts>, DefaultIgnore; + ---------------- xazax.hun wrote: > 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. I also prefer a cc1 flag -- using warnings to control debugging behavior is novel and doesn't seem like something we should encourage. I don't think you would need to pass any additional flags to Sema however; I would expect this to show up in a language option so it can be queried through `LangOpts`. This makes it available to other things, like clang-tidy checks as well. ================ Comment at: clang/lib/AST/LifetimeAttr.cpp:23 + const auto *Ctor = CE->getConstructor(); + if (Ctor->getParent()->getName() == "PSet") + return CE; ---------------- This need some explanation as it sort of comes out of thin air. What is special about a class named `PSet`? Does it need to be in the global namespace, or a class named `PSet` in any namespace is special? ================ Comment at: clang/lib/AST/LifetimeAttr.cpp:123 + if (ULE->getName().isIdentifier() && + ULE->getName().getAsIdentifierInfo()->getName() == "lifetime") + break; ---------------- Same question about identifier namespaces here as above. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4516 + +static void handleLifetimeContractAttr(Sema &S, Decl *D, const ParsedAttr &AL) { + LifetimeContractAttr *LCAttr; ---------------- I suspect you want this to handle redeclarations where you may be working with an inherited attribute? If so, you probably should be following the `merge` pattern used by other attributes. ================ Comment at: clang/lib/Sema/SemaType.cpp:7727 + // Move function type attribute to the declarator. + case ParsedAttr::AT_LifetimeContract: ---------------- 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. ================ Comment at: clang/test/Sema/attr-psets-annotation.cpp:1 +// NOT RUN: %clang_cc1 -fsyntax-only -Wlifetime -Wlifetime-dump-contracts -verify %s + ---------------- I think this test should be in SemaCXX instead of Sema. ================ Comment at: clang/test/Sema/attr-psets-annotation.cpp:44 +void basic(int *a, int *b) [[gsl::pre(gsl::lifetime(b, {a}))]]; +// expected-warning@-1 {{Pre { b -> { a }; }}} + ---------------- Please spell out the full diagnostic the first time it is used in the test file. ================ Comment at: clang/test/Sema/attr-psets-annotation.cpp:130 +// Warnings/errors + +void unsupported_contract(int *a, int *b) [[gsl::pre(gsl::lifetime(b, {a++}))]]; ---------------- This is missing all of the tests which verify the sanity of the attribute (appertains to the correct subject, accepts the correct number/type of arguments, etc). 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