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