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

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

I'll commit this shortly.



================
Comment at: llvm/lib/IR/DIBuilder.cpp:351
                                         DIScope *Context, uint32_t AlignInBits,
+                                        DINode::DIFlags Flags,
                                         DINodeArray Annotations) {
----------------
J-Camilleri wrote:
> 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.
If you don't have commit rights, no worries - I can sort this out when I commit this on your behalf.
But yes, your list of the two steps/order/pieces is correct.


================
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:
----------------
J-Camilleri wrote:
> 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`.
Ah, makes sense - thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134339