r281278 - [DebugInfo] Deduplicate debug info limiting logic

[Reid Kleckner via cfe-commits]

On Mon, Sep 12, 2016 at 5:20 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Mon, Sep 12, 2016 at 5:09 PM Reid Kleckner via cfe-commits <
> cfe-commits at lists.llvm.org> 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
>> Log:
>> [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
some reason.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160919/c868a6f5/attachment.html>