[PATCH] Fully handle globals and functions in CGDebugInfo::getDeclarationOrDefinition()

Frederic Riss friss at apple.com
Mon Nov 17 12:13:53 PST 2014


>>! In D6173#5, @dblaikie wrote:
> Looks pretty reasonable. Some optional and/or follow-up patch comments.

I'll commit that with your feedback addressed.

> Have you considered if/how some of this could be unified with the type decl-or-def handling?

I kinda did, but there were some unclear points that made me chose the separate handling approach. Maybe you can shed some light on those:
 - The TypeCache is keyed on the TagType itself. Unifying the map would mean keying on the canonical decl. I /think/ this should be fine, but I couldn't convince myself that I'm not missing something.
 - In the finalize phase, the type replacement logic asserts that there always is a replacement type registered in the TypeCache, while for decls there might not be one. I'm not actually sure where the guarantee comes from on the type side.

================
Comment at: lib/CodeGen/CGDebugInfo.cpp:2310
@@ -2309,1 +2309,3 @@
 
+void CGDebugInfo::collectFunctionDeclProps(GlobalDecl GD,
+                                           llvm::DIFile Unit,
----------------
dblaikie wrote:
> dblaikie wrote:
> > If you want to submit these refactorings separately, feel free (& do that sort of thing without precommit review - just pulling out a bit of functionality and giving it a name is usually goodness even before the second user exists (we tend to have long functions that are hard to read - pulling out parts of them and giving them names usually makes them a bit easier to follow) - and it makes the code reviews more palatable (I can easily glance at a single function refactor like this, and mostly just assume you did the obvious thing of copy/paste the code and give it a name))
> Though the name 'collect' might be different from the existing similar functions we have for subprograms - could you check if we can have a more consistent naming one way or the other? (I think I named the subprogram ones 'apply' rather than 'collect')
> 
> Oh... I was confused, thought this was an LLVM patch (since it was sent to llvm-commits) so I was talking about LLVM codegen/asmprinter stuff... 
> 
> Nevermind.
C'mon I did this mailing list mistake again... Sorry for the confusion.

http://reviews.llvm.org/D6173






More information about the llvm-commits mailing list