dblaikie added a comment.
Looks pretty good to me - if you could commit the debug info level refactoring
separately/up-front, and maybe the test case could be simplified a bit. Looking
forward to seeing what comes of this option, analysis of missing types, etc.
================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:360-364
+
+ /// Check if full debug info should be emitted.
+ bool isFullDebug() const {
+ return getDebugInfo() >= codegenoptions::DebugInfoConstructor;
+ }
----------------
dblaikie wrote:
> Could you commit (the pre-patch equivalent) of this change now/before your
> patch - it'll simplify the diff/make it easier to review.
>
> Though I think the name needs some massaging, since it doesn't return
> "getDebugInfo() == codegenoptions::FullDebugInfo", so the name's a bit
> misleading.
>
> /maybe/ (but honestly I don't have any great names)
> "hasVariableAndTypeDebugInfo" though that's a bit of a mouthful.
Yeah, name's still a bit awkward, but I've got nothing better - could you
submit this function/usage now/ahead of the rest of this patch so it's easier
to see what this patch is changing? (& easier to revert this patch if needed,
etc)
================
Comment at: clang/test/CodeGenCXX/debug-info-limited-ctor.cpp:9-15
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "B"
+// CHECK-SAME: flags: DIFlagFwdDecl
+struct B {};
+int bar(B *b);
+int TestB(B *b) {
+ return bar(b);
+}
----------------
This test case doesn't look interesting to me - I would expect that'd be
covered by other tests already in-tree?
Is there a particular nuance this case is intended to capture? (I'm curious
why/how 'bar's presence would make a difference in the presence of TestB
already)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72427/new/
https://reviews.llvm.org/D72427
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
