[PATCH] D148381: [WIP][Clang] Add counted_by attribute

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:4541-4544
+bool FieldDecl::isFlexibleArrayMemberLike(
+    ASTContext &Ctx,
+    LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel,
+    bool IgnoreTemplateOrMacroSubstitution) const {
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > There's a lambda in `isUserWritingOffTheEnd` in clang/lib/AST/ExprConstant.cpp assigned to a variable `isFlexibleArrayMember`.  Any way we can reuse code between here and there if we host that into a proper method somewhere else?
> s/host/hoist/
Maybe. The one in `ExprConstant.cpp` looks like it's used in a slightly different way. I'll check it out, but might do it later on...


================
Comment at: clang/lib/AST/Decl.cpp:4590
+      if (ConstantArrayTypeLoc CTL = TL.getAs<ConstantArrayTypeLoc>()) {
+        const Expr *SizeExpr = dyn_cast<IntegerLiteral>(CTL.getSizeExpr());
+        if (!SizeExpr || SizeExpr->getExprLoc().isMacroID())
----------------
nickdesaulniers wrote:
> `const Expr *` on LHS of assignment, `dyn_cast<IntegerLiteral>` on RHS. What is going on here?
Unknown. This is copy-pasted from the `Expr.cpp` version...I'll see if we can do without the cast.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:975-976
+    if (const auto *RD = Ty->getAsRecordDecl())
+      for (const FieldDecl *Field : RD->fields())
+        VD = Field;
+  } else if (const auto *CE = dyn_cast<CastExpr>(Base)) {
----------------
nickdesaulniers wrote:
> Why is this re-assigning VD? Is this trying to get the last field in the RecordDecl?
> 
> Can you use `field_end()` or `--field_end()`?
> 
> In fact I see this pattern a lot in this patch. Time for a new method on RecordDecl? `const FieldDecl *RecordDecl::getLastField()`?
Created the method. We can't use `field_end` or `--field_end` because it's a forward iterator...


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:992-996
+  auto It = llvm::find_if(FD->getParent()->fields(),
+                          [&](const FieldDecl *Field) {
+                              return FieldName == Field->getName();
+                          });
+  return It != FD->getParent()->field_end() ? *It : nullptr;
----------------
nickdesaulniers wrote:
> Can `llvm::is_contained` from llvm/ADT/STLExtras.h help here?
I don't think that works. I thin it requires that the element we search for is of the same type. Here we have only  its name. There might be a nicer way of doing this though.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17914-17915
+    if (const auto *SubRD = dyn_cast<RecordDecl>(D))
+      if (const FieldDecl *FD = FindFieldWithCountedByAttr(SubRD))
+        return FD;
+  }
----------------
nickdesaulniers wrote:
> I think the `if` is unnecessary, could simply `return FindFieldWithContedByAttr(SubRD);`?
Nice catch.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17926-17930
+  auto It = llvm::find_if(
+      RD->fields(), [&](const FieldDecl *Field){
+        return Field->getName() == ECA->getCountedByField()->getName();
+      });
+  if (It == RD->field_end()) {
----------------
nickdesaulniers wrote:
> `llvm::is_contained`?
It's a similar issue as the one above: the ECA returns an `IdentifierInfo`, but `fields()` has only `FieldDecls`.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:18001-18003
+      const IdentifierInfo *Unknown = CheckCountedByAttr(RD, FD, SR);
+
+      if (Unknown)
----------------
nickdesaulniers wrote:
> does it fit on one line to do:
> 
> ```
> if (const IdentifierInfo *I = CheckCountedByAttr(RD, FD, SR))
> ```
Yeah. I must have missed that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381