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

[Jonathan Camilleri via Phabricator via llvm-commits]

J-Camilleri marked 3 inline comments as done.
J-Camilleri added inline comments.


================
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;
+
----------------
dblaikie wrote:
> 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.
I agree with your assesment. I changed the test to just check that type aliases emit the flag not the logic of which flag is emitted.


================
Comment at: llvm/lib/IR/DIBuilder.cpp:351
                                         DIScope *Context, uint32_t AlignInBits,
+                                        DINode::DIFlags Flags,
                                         DINodeArray Annotations) {
----------------
dblaikie wrote:
> 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.
Regarding this and the previous comment about separating the llvm commit.

Am I to have 2 commits as follows?
1. change llvm to emit flag + test case
2. modify llvm interface + change clang to emit flag + test case

I do not have merge rights so I will ask for this when I get to the merge stage.

Thanks for the review.


================
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:
----------------
dblaikie wrote:
> 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.
I left the llvm to be the same as generated by the cpp file but changed the `CHECK` to just verify that the accessibility flag is emitted for a `typedef`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134339