aaron.ballman added a comment. Why did this get committed when there were still outstanding review questions that were not answered?
In D69759#1739935 <https://reviews.llvm.org/D69759#1739935>, @yonghong-song wrote: > Regarding to this one "This is missing a bunch of C++ tests that show what > happens in the case of inheritance, template instantiations, etc." > Could you give me some example tests which I can take a look in order to add > support. FYI, BPF backend publicly only supports C language, > (a restrict C for example __thread keyword is not supported). I guess > template instantiations does not apply here? Or we can still test > template instantiation since we are at clang stage? If the attribute only makes sense in C mode, it should not be accepted in C++ mode. However, it currently is accepted in C++ mode which is why I was asking for tests demonstrating how it should behave. In D69759#1742264 <https://reviews.llvm.org/D69759#1742264>, @yonghong-song wrote: > @aaron.ballman just ping, could you let me know if you have any further > comments? Thanks! I was out all last week at wg21 meetings and a short vacation, so that's why the delay in review. FWIW, we usually give a week before pinging a review. ================ Comment at: clang/include/clang/Basic/Attr.td:1584 + TargetSpecificAttr<TargetBPF> { + let Spellings = [Clang<"preserve_access_index">]; + let Subjects = SubjectList<[Record], ErrorDiag>; ---------------- If this attribute is only supported in C mode, then this attribute should have: `let LangOpts = [COnly];` in it. ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:3411-3422 + const auto *ImplicitCast = dyn_cast<ImplicitCastExpr>(ArrayBase); + if (!ImplicitCast) + return false; + + // Only support base as either a MemberExpr or DeclRefExpr. + // DeclRefExpr to cover cases like: + // struct s { int a; int b[10]; }; ---------------- Is there a reason to do this as opposed to `IgnoreImpCasts()`? ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:3423-3424 + const Expr *E = ImplicitCast->getSubExpr(); + const auto *MemberCast = dyn_cast<MemberExpr>(E); + if (MemberCast) + return MemberCast->getMemberDecl()->hasAttr<BPFPreserveAccessIndexAttr>(); ---------------- ``` if (const auto *ME = dyn_cast<MemberExpr>(E)) return ... ``` ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:3427-3428 + + const auto *DeclRefCast = dyn_cast<DeclRefExpr>(E); + if (DeclRefCast) { + const VarDecl *VarDef = dyn_cast<VarDecl>(DeclRefCast->getDecl()); ---------------- Similar here -- you can sink the variable into the `if`. ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:3433 + + const auto *PtrT = dyn_cast<PointerType>(VarDef->getType().getTypePtr()); + if (!PtrT) ---------------- `const auto *PtrT = VarDef->getType()->getAs<PointerType>();` ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:3440-3443 + while (TypedefT) { + PointeeT = TypedefT->desugar().getTypePtr(); + TypedefT = dyn_cast<TypedefType>(PointeeT); + } ---------------- Is there a reason you only want to strip off typedefs, and not other forms of sugar? e.g., why not call `getUnqualifiedDesugaredType()`? ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:3446 + // Not a typedef any more, it should be an elaborated type. + const auto ElaborateT = dyn_cast<ElaboratedType>(PointeeT); + if (!ElaborateT) ---------------- `const auto *` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69759/new/ https://reviews.llvm.org/D69759 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits