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