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

Reply via email to