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