[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This is missing a bunch of C++ tests that show what happens in the case of inheritance, template instantiations, etc.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10068
   "__builtin_preserve_field_info argument %0 not a constant">;
+def err_preserve_access_index_wrong_type: Error<"%0 attribute only applies to %1">;
 
----------------
This is unnecessary.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5706
+  // Add preserve_access_index attribute to all fields and inner records.
+  for (DeclContext::decl_iterator D = RD->decls_begin(), DEnd = RD->decls_end();
+       D != DEnd; ++D) {
----------------
Any reason not to use a range-based for?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5712
+
+    RecordDecl *Rec = dyn_cast<RecordDecl>(*D);
+    if (Rec) {
----------------
Can use `auto` here


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5713-5718
+    if (Rec) {
+      Rec->addAttr(::new (S.Context) BPFPreserveAccessIndexAttr(S.Context, AL));
+      handleBPFPreserveAIRecord(S, Rec, AL);
+    } else {
+      D->addAttr(::new (S.Context) BPFPreserveAccessIndexAttr(S.Context, AL));
+    }
----------------
These should be implicit attributes, not explicit ones, right?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5725
+  // Add preserve_access_index attribute to all fields and inner records.
+  for (DeclContext::decl_iterator D = RD->decls_begin(), DEnd = RD->decls_end();
+       D != DEnd; ++D) {
----------------
Similar here.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5727
+       D != DEnd; ++D) {
+    RecordDecl *Rec = dyn_cast<RecordDecl>(*D);
+    if (Rec) {
----------------
Can use `auto` here as well.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5730-5736
+      if (!Rec->hasAttr<BPFPreserveAccessIndexAttr>()) {
+        Rec->addAttr(::new (S.Context) BPFPreserveAccessIndexAttr(S.Context, AL));
+        handleBPFPreserveAIRecord(S, Rec, AL);
+      }
+    } else {
+      D->addAttr(::new (S.Context) BPFPreserveAccessIndexAttr(S.Context, AL));
+    }
----------------
Implicit attributes?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5742-5747
+  RecordDecl *Rec = dyn_cast<RecordDecl>(D);
+  if (!Rec) {
+    S.Diag(D->getLocation(), diag::err_preserve_access_index_wrong_type)
+        << "preserve_addess_index" << "struct or union type";
+    return;
+  }
----------------
This should be handled in Attr.td with an explicit subject.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5749
+
+  if (!checkAttributeNumArgs(S, AL, 0))
+    return;
----------------
This is not needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69759/new/

https://reviews.llvm.org/D69759