These will be used to implement support for useful forward declarartions.
Details
Diff Detail
Event Timeline
lib/IR/DIBuilder.cpp | ||
---|---|---|
1032 | Should this be hardcoded as a definition? Seems like the "createTempStaticVariableFwdDecl" would want a declaration, not a definition. | |
1185 | Should this take "isDefinition"? The function name indicates that it's only a declaration. | |
1207 | Wonder if there's a nicer way to do this - templated & pass in a lambda? But it's nothing drastic either way. |
lib/IR/DIBuilder.cpp | ||
---|---|---|
1032 | Doh! you're right. I had it and then reworked the patch and lost that change... | |
1207 | I had a /simpler/ way, but I'm not sure you'll find it nicer (In fact I changed it because I thought you'd not like it). In the first incarnation of the patch, I just checked the isDefinition value and decided upon temporary or not based on that. I audited all users and if I didn't miss anything, none passes false for isDefinition today. Thus only the ones I will introduce would need to care about the temporary aspect. And similarly for the variables (where I introduced a new argument for isDefinition that I lost in this version of the patch). |
lib/IR/DIBuilder.cpp | ||
---|---|---|
1041 | How's this work? What will we do at the finalize step? | |
1207 | Yeah - the only function declarations we create currently are member functions for which we use "createMethod" - though that's a subtle distinction that's probably not ideal to rely on (one day we might decide that createMethod can go away & we can just create declarations - declarations that would never be replaced with definitions). What about passing in a function pointer? (&MDNode::getTemporary or &MDNode::get) Avoids a boolean argument (which is always a bit unclear at a call site without a comment) except we need to do other work (AllSubprograms and AllGVs)... I guess we could rely on the fact that we only emit declarations when there's something that refers to them (so they don't necessarily need to be in retention lists), but that's still pretty subtle. |
lib/IR/DIBuilder.cpp | ||
---|---|---|
1041 | I realise I hadn't replied to this. In followup patches, in CGDebugInfo::finalize(), forward declarations which have seen definitions are RAUWed and dropped, the other ones are placed into the corresponding retention list. They need to be in retention lists, not because thy would be lost otherwise, but simply because these lists are emitted first in DwarfUnit. And the imported_declaration stuff expects that all the referenced DIEs are already emitted at the time they are processed. | |
1207 | I agree regarding the general meaning of a boolean at the call site. The function pointer would be nice if we didn't do any special processing depending on the value, but I thing testing for a function pointer of a certain value would look odd. And regarding not putting the declarations in the retention lists, I replied above. What about your first suggestion of a lambda? It would be pretty explicit. |
Addressed review comments:
- add isDefinition parameter to createStaticVariable
- use a std::function and lambdas to convey the intent of the caller into createFunctionHelper and createStaticVariableHelper
lib/IR/DIBuilder.cpp | ||
---|---|---|
1041 | Cna we just fix the imported_declaration stuff so it doesn't expect all the referenced DIEs to be already emitted (it can use the usual "getOrCreate" routines that lazily construct stuff if it's not already been constructed). Also you said: " forward declarations which have seen definitions are RAUWed and dropped, the other ones are placed into the corresponding retention list." - they'll still need to be RAUW'd with themselves to promote them from temporary to non-temporary. I imagine this should look much the same as the code we already have for handling temporary type forward declarations, no? I'd expect to see it pretty much exactly the same/symmetric - is that not going to be the case? | |
1173–1179 | Since this is a static function, you can make it a template and avoid the std::function dynamic overhead - but I suppose that's a binary size V execution time tradeoff... I don't feel too strongly about it either way, though I'd probably have gone with a template rather than std::function, personally. | |
1248 | Not sure what the style guide says, but I usually just capture by "[&]" rather than listing the me explicitly (goes for here and above) |
lib/IR/DIBuilder.cpp | ||
---|---|---|
1041 | I'll try to look into the imported DIE emission code to see if we can remove constraints. It's not exactly symmetric, but it looks pretty much the same. For example, in CGDebugInfo::finalise(), the loop handling the type expects that every type in the ReplaceMap has a counterpart in the TypeCache (I'm not totally clear how this is always true). For functions and variables, the handling loop allows definitions not to exist, but it's basically the same. | |
1173–1179 | Can do. I find the std::function more self documenting, but it has a sometimes noticeable overhead. I didn't think it would show up on profiles here, but if you prefer I'll happily change that to a template parameter. | |
1248 | I haven't tried it, but I think [&] wouldn't capture 'this', as it can't be captured by reference. And wouldn't capturing everything by value create a lot of useless copies (in other words, does the compiler only copy what is needed?). |
Should this be hardcoded as a definition? Seems like the "createTempStaticVariableFwdDecl" would want a declaration, not a definition.