[PATCH] D148381: [WIP][Clang] Add counted_by attribute
Bill Wendling via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 22 18:09:33 PDT 2023
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
More information about the cfe-commits
mailing list