[PATCH] D134339: [clang][llvm] generate accessibility metadata for type aliases

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Looks pretty good to me - few things to clean up/simplify.



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1331-1333
+  if (isa<RecordDecl>(DC)) {
+    Flags = getAccessFlag(Ty->getDecl()->getAccess(), cast<RecordDecl>(DC));
+  }
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: clang/test/CodeGenCXX/debug-info-access.cpp:23-27
+
+  // CHECK-DAG: !DIDerivedType(tag: DW_TAG_typedef, name: "pub_typedef",{{.*}} line: [[@LINE+1]],{{.*}} flags: DIFlagPublic)
+  typedef int pub_typedef;
+  pub_typedef pub_member;
+
----------------
Arguably/I'd be fine with testing just one case here - since the implementation of choosing when to put an access modifier, which modifier to put, etc, is is already implemented and tested for other cases - so we might not need to duplicate that testing for this case. 

But I don't mind too much either way.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:793-795
+
+  addAccess(Buffer, DTy->getFlags());
+
----------------
This piece and the llvm test case should go in a separate commit (it can be committed before or after the clang-centric commit (probably before makes more sense)) but we can review it all together.


================
Comment at: llvm/lib/IR/DIBuilder.cpp:351
                                         DIScope *Context, uint32_t AlignInBits,
+                                        DINode::DIFlags Flags,
                                         DINodeArray Annotations) {
----------------
While we usually split llvm & clang commits, in this case since it changes an API used by clang, this part of the llvm change should go along with the clang change.


================
Comment at: llvm/test/DebugInfo/X86/debug-info-access.ll:12-32
+;
+;     using pub_default_using = int;
+;     pub_default_using a_pub;
 ;   };
 ;
 ;   class B : public A {
 ;   public:
----------------
Probably don't need all this test coverage - since most of this is motivated by the features up in clang - on the LLVM side we just need one case that demonstrates that if we put an access specifier in the flags, that gets emitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134339