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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
