On Mon, Sep 12, 2016 at 5:20 PM, David Blaikie <dblai...@gmail.com> wrote:
> On Mon, Sep 12, 2016 at 5:09 PM Reid Kleckner via cfe-commits <
> email@example.com> wrote:
>> Author: rnk
>> Date: Mon Sep 12 19:01:23 2016
>> New Revision: 281278
>> URL: http://llvm.org/viewvc/llvm-project?rev=281278&view=rev
>> [DebugInfo] Deduplicate debug info limiting logic
>> We should be doing the same checks when a type is completed as we do
>> when a complete type is used during emission. Previously, we duplicated
>> the logic, and it got out of sync. This could be observed with
>> dllimported classes.
> Thanks for having a go at this!
> The logic in completeType (& maybe also completeClassData) still seems to
> duplicate some of the logic in shouldOmitDefinition - I think the original
> idea I had was a series of cascading entry points (completeType checking
> the "oh, the type now has a definition", then falling into
> completeRequiredType, etc)
> In theory the "shouldOmitDefinition" could be split into the extra
> conditions for each of the parts of that cascade, so the checks wouldn't be
> redundantly executed when we were already being called about the completion
> or required completion of a type, etc. But I'm not sure the performance
> gain would be measurable, etc.
> If not, we might as well just have one entry point that's something like
> "revisit type because something interesting happened to it" (it became
> complete, it became required to be complete, etc)... but that seems a bit
> weird too.
Seems reasonable, but I didn't see a nice, clear, obvious refactoring to
do, so I left it here. Seems better than it was before.
>> Also reduce a test case for this slightly.
> The UseCompleteType is still there/unnecessary (as is the 'private' in
> UnicodeString) - and I'd probably rename it to foo/bar and throw it in a
> namespace like the rest for consistency - having semantic names in a test
> when they're just remnants from whatever code it was reduced from seems
> confusing to me. But I realize I'm a bit more pedantic about that than most.
The private was necessary at some point during the reduction process for
cfe-commits mailing list