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