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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

I'd also appreciate if you updated the docs for the changes done in this patch.



================
Comment at: clang/include/clang/AST/LifetimeAttr.h:22
+
+/// This represents an abstract memory location that is used in the lifetime
+/// contract representation.
----------------
"/// An abstract memory location..."

It would also really help to show some examples of the sorts of locations it can express (especially the nested field accesses).


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:24
+/// contract representation.
+class ContractVariable {
+public:
----------------
I feel like the name is too broad, maybe LifetimeContractVariable?


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:31
+
+  ContractVariable(const RecordDecl *RD) : RD(RD), Tag(This) {}
+
----------------
I'd suggest to make the above two constructors into named factories as well. It took me a while to realize that converting a RecordDecl* to ContractVariable actually models a "this" pointer.


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:71
+
+  bool isThisPointer() const { return Tag == This; }
+
----------------
I'd suggest to add all such `is*` methods in this commit, and group them next to each other in the source code.


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:143
+using LifetimeContractSet = std::set<ContractVariable>;
+using LifetimeContractMap = std::map<ContractVariable, LifetimeContractSet>;
+
----------------
These names are not descriptive...


================
Comment at: clang/include/clang/AST/LifetimeAttr.h:146
+namespace process_lifetime_contracts {
+// This function and the callees are have the sole purpose of matching the
+// AST that describes the contracts. We are only interested in identifier names
----------------
I'm sorry, but this comment is missing the most important piece of information -- what this function does.

This comment says "matching the AST that describes the contracts", which does give me a feeling of the compiler component, but I have no idea about how exactly it uses the expression that is passed in, and what happens with the map, and why a SourceRange is returned.


================
Comment at: clang/include/clang/Basic/Attr.td:2897
+  let Subjects = SubjectList<[Function]>; // TODO: HasFunctionProto?
+  let Args = [ExprArgument<"Expr">];
+  let LateParsed = 1;
----------------
I'd suggest to call it something other than "Expr" (to avoid confusion with or shadowing the type name) -- "ContractExpr" maybe?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3342
+  InGroup<LifetimeAnalysis>, DefaultIgnore;
+def warn_dump_lifetime_contracts : Warning<"%0">, InGroup<LifetimeDumpContracts>, DefaultIgnore;
+
----------------
I think a warning that is a debugging aid is new territory.


================
Comment at: clang/lib/AST/LifetimeAttr.cpp:16
+namespace process_lifetime_contracts {
+// Easier access the attribute's representation.
+
----------------
I don't think this comment adds much.


================
Comment at: clang/lib/AST/LifetimeAttr.cpp:18
+
+static const Expr *ignoreReturnValues(const Expr *E) {
+  const Expr *Original;
----------------
I'm not sure what you mean by "return values".

`ignoreWrapperASTNodes`?


================
Comment at: clang/lib/AST/LifetimeAttr.cpp:55
+    else if (Name == "Static")
+      return LifetimeContractSet{ContractVariable::staticVal()};
+    else if (Name == "Invalid")
----------------
The lifetime paper says it is called "global" now:

> Renamed static to global to reduce confusion with the keyword.

Also, the paper spells these special names in all lowercase letters.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4532
+  if (ErrorRange.isValid())
+    S.Diag(ErrorRange.getBegin(), diag::warn_unsupported_expression)
+        << ErrorRange;
----------------
I'm assuming higher quality error messages are going to come later? Right now it is a catch-all message that is shown for unsupported expressions, expressions that don't type check, and expressions that will never be supported.


================
Comment at: clang/test/Sema/attr-psets-annotation.cpp:24
+struct PointerTraits {
+  static auto deref(const T &t) -> decltype(*t);
+};
----------------
If this code imitates an existing library, please ignore.

Declaring PointerTraits::deref to be a function and then invoking it and using decltype on the return value seems unnecessarily complex.

You could declare a type alias within PointerTraits instead.

```
template<typename T>
struct PointerTraits {
  using DerefType = decltype(*declvar<T>());
};

template<typename T>
struct PointerTraits<T*> {
  using DerefType = T;
}; // Is this specialization needed? Wouldn't the primary template work?

template<typename T>
struct PointerTraits<T&> {
  using DerefType = T;
};
```

Also, a specialization for rvalue references?

However, I'm not sure if the specialization even for regular references will be used -- the free function deref() strips the reference from T (also strips constness from it!) Due to reference collapse rules, T will never be a reference.


================
Comment at: clang/test/Sema/attr-psets-annotation.cpp:95
+  void f(X **out)
+      [[gsl::post(lifetime(deref(out), {this}))]];
+  // expected-warning at -2 {{Pre { }  Post { *out -> { this }; }}}
----------------
I'm not sure what causes this unqualified call to `deref` to call `gsl::deref` (in a different namespace). Does not seem like ADL would do it, since the type of `out` is also not in `gsl`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72810