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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:4170
+    private:
+      mutable SmallVector<SourceRange, 2> CountFieldRanges;
+    public:
----------------
`mutable`...my least favorite keyword in C++.  If you drop `const` from `addCountFieldSourceRange` then you don't need `mutable`.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:7010
+    /* ... */
+    struct bar *fam[] __attribute__((element_count(bork, num_fam_elements)));
+  };
----------------
Question: does `bork` need to occur before `fam` for this substructure reference to work?

Does that mean that `element_count` is essentially variadic with this syntax?


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:954
+        if (const auto *MD = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
+          if (const auto ECA = MD->getAttr<ElementCountAttr>()) {
+            const RecordDecl *RD = MD->getParent();
----------------
`const auto *ECA = ...`


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:961-963
+              for (FieldDecl *FD : RD->fields()) {
+                if (FD->getName() != CountField->getName())
+                  continue;
----------------
What happens if we never find the field? I guess that's checked below in `CheckElementCountAttr`? Do we need a diagnostic though?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17689-17701
+static const FieldDecl *FindFieldWithElementCountAttr(const RecordDecl *RD) {
+  for (const Decl *D : RD->decls()) {
+    if (const auto *FD = dyn_cast<FieldDecl>(D))
+      if (FD->hasAttr<ElementCountAttr>())
+        return FD;
+
+    if (const auto *SubRD = dyn_cast<RecordDecl>(D))
----------------
I think if you made this static function instead be a method of RecordDecl, you could reuse it in clang/lib/CodeGen/CGExpr.cpp (in that for loop where you iterate `RD->fields`.

`FieldDecl *RecordDecl::getFieldWithName(StringRef Name);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381