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

[Bill Wendling via Phabricator via cfe-commits]

void marked an inline comment as done.
void added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:4170
+    private:
+      mutable SmallVector<SourceRange, 2> CountFieldRanges;
+    public:
----------------
nickdesaulniers wrote:
> `mutable`...my least favorite keyword in C++.  If you drop `const` from `addCountFieldSourceRange` then you don't need `mutable`.
Hmm...I needed it at one point, but it no longer looks like I do.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:7010
+    /* ... */
+    struct bar *fam[] __attribute__((element_count(bork, num_fam_elements)));
+  };
----------------
nickdesaulniers wrote:
> 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?
It doesn't need to be before the FAM, but I think a flexible array member needs to be at the end of a structure (I think anyway).

And yes, it's variadic. I'll need to expand that in the documentation, but really the documentation should be reworked a lot.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8768
 class AttrImporter {
+  friend ASTImporter;
+
----------------
balazske wrote:
> This line is not necessary.
I need access to the `ToAttr` member. I can add an accesser though.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8987
+    cast<ElementCountAttr>(FromAttr)->copyCountFieldSourceRanges(
+        cast<ElementCountAttr>(AI.ToAttr));
+    break;
----------------
balazske wrote:
> From `ASTImporter` point of view this looks not correct: The `ToAttr` has a different AST context than the `FromAttr`, simple copy of a source location is not enough. There is a `ASTImporter::Import(SourceRange)` function that can be used.
Ah! Thanks.


================
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();
----------------
nickdesaulniers wrote:
> `const auto *ECA = ...`
Doh!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381