[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

[Bob Haarman via Phabricator via cfe-commits]

inglorion added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:45
+  }
+  return IDL;
+}
----------------
rnk wrote:
> Does MSVC accept this? I think it will emit the copy ctor call in an -O0 build.
I wrote this thinking that the right thing would happen under copy elision (there is only one object, move constructor isn't called, and the destructor only runs once) and without copy elision (there are two objects, move constructor is called, destructor is run for both objects but is a no-op for the moved-from object). If that's not the case, how would you rewrite this to do the right thing?


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:145
 
   case Stmt::IfStmtClass:       EmitIfStmt(cast<IfStmt>(*S));             break;
   case Stmt::WhileStmtClass:    EmitWhileStmt(cast<WhileStmt>(*S));       break;
----------------
rnk wrote:
> Doesn't this end up recursing? Won't InhibitDebugLocation prevent us from applying the inner statement locations?
Yeah, this looks wrong. Let me get back to you with fixed code or an explanation of why it actually does the right thing.


================
Comment at: clang/test/CodeGenCXX/debug-info-nested-exprs.cpp:12
+
+// NONEST: call i32 @{{.*}}bar{{.*}}, !dbg ![[LOC:[0-9]+]]
+// NONEST: call i32 @{{.*}}baz{{.*}}, !dbg ![[LOC]]
----------------
rnk wrote:
> This is pretty painful to test. :(
> 
> If we let ourselves do an asm test, .cv_loc is printed with some nice assembly comments that make this testing easy.
Would you like me to write an asm test in addition to / instead of this one?


================
Comment at: clang/test/CodeGenCXX/debug-info-nested-exprs.cpp:44
+  int a = bar(x, y) +
+          baz(x, z) +
+          qux(y, z);
----------------
zturner wrote:
> Can you make a function called `int foo()` and make this `int a = bar(foo(), y) + ...`
Yes. Why? To test an additional level of nesting?


https://reviews.llvm.org/D37529