On Mon, Jun 23, 2014 at 1:39 PM, Diego Novillo <[email protected]> wrote: > > > > On Mon, Jun 23, 2014 at 4:30 PM, David Blaikie <[email protected]> wrote: >> >> Looks good - though I'd prefer to separate the discussion about column >> info so as to isolate the changes/tests/etc and not delay this review. >> >> ================ >> Comment at: lib/Frontend/CompilerInvocation.cpp:571 >> @@ +570,3 @@ >> + Opts.setDebugInfo(CodeGenOptions::LocTrackingOnly); >> + Opts.DebugColumnInfo = true; >> + } >> ---------------- >> Perhaps we could leave this out for now & just have a separate >> CR/discussion about the semantics/ramifications of having this difference >> between -R and -gmlt? > > > Hm, perhaps. I would be penalizing -Rpass, however. But I guess it doesn't > matter much. I'll amend the documentation to state that -gcolumn-inf is > needed with -Rpass.
Well it's not worse - currently -Rpass doesn't enable debug info at all , but I suppose it at least suggests the user enable both -gmlt and -gcolumn-info. But I assume if they enabled -g(mlt or otherwise) they wouldn't be told they should use -gcolumn-info. So, committing this patch without the column info would be sort of a regression, but sort of not... Given this is a developer feature only, I'm OK with it being a little rough around the edges. Up to you though, we can discuss the issues with -gcolumn-info and -Rpass in this review if you prefer. Just figured it'd be better to separate them if practical. >> ================ >> Comment at: test/Frontend/optimization-remark.c:13 >> @@ +12,3 @@ >> +// CHECK: , !dbg ! >> +// CHECK-NOT: DW_TAG_base_type >> + >> ---------------- >> Could have a brief comment here "Ensure -Rpass, like -gmlt, doesn't >> produce debug info metadata for types". > > > That's what the comment above those two lines tries to do. Perhaps you want > it worded differently? Oh, no - that's fine. I'm just a bit myopic. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
