[PATCH] D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes.

[David Blaikie via Phabricator via cfe-commits]

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