nickdesaulniers added a comment.
Drive by code review!!!
================
Comment at: clang/lib/AST/Decl.cpp:4541-4544
+bool FieldDecl::isFlexibleArrayMemberLike(
+ ASTContext &Ctx,
+ LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel,
+ bool IgnoreTemplateOrMacroSubstitution) const {
----------------
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?
================
Comment at: clang/lib/AST/Decl.cpp:4589
+
+ if (ConstantArrayTypeLoc CTL = TL.getAs<ConstantArrayTypeLoc>()) {
+ const Expr *SizeExpr = dyn_cast<IntegerLiteral>(CTL.getSizeExpr());
----------------
auto
================
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())
----------------
`const Expr *` on LHS of assignment, `dyn_cast<IntegerLiteral>` on RHS. What is
going on here?
================
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)) {
----------------
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()`?
================
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;
----------------
Can `llvm::is_contained` from llvm/ADT/STLExtras.h help here?
================
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;
+ }
----------------
I think the `if` is unnecessary, could simply `return
FindFieldWithContedByAttr(SubRD);`?
================
Comment at: clang/lib/Sema/SemaDecl.cpp:17921-17922
+
+static const IdentifierInfo *
+CheckCountedByAttr(const RecordDecl *RD, const FieldDecl *FD,
+ SourceRange &Loc) {
----------------
consider adding a comment above this function that it returns nullptr on success
================
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()) {
----------------
`llvm::is_contained`?
================
Comment at: clang/lib/Sema/SemaDecl.cpp:18001-18003
+ const IdentifierInfo *Unknown = CheckCountedByAttr(RD, FD, SR);
+
+ if (Unknown)
----------------
does it fit on one line to do:
```
if (const IdentifierInfo *I = CheckCountedByAttr(RD, FD, SR))
```
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
