[PATCH] D36386: [clang] Remove unit test which uses reverse-iterate and fix a PointerLikeTypeTrait specialization

[David Blaikie via llvm-commits]

OK - go ahead and remove the tests then, if the functionality is now
covered by the buildbot+existing tests.

On Wed, Aug 9, 2017 at 12:04 PM Grang, Mandeep Singh <mgrang at codeaurora.org>
wrote:

> > Ah, OK. I'm still curious about whether this results in a loss of test
> coverage. Without this test, would the bug it was testing still be caught
> by some test failure in at least one of the forward or reverse iteration
> modes?
>
> Sorry ... I missed that. Yes, the reverse iteration buildbot
> http://lab.llvm.org:8011/builders/reverse-iteration would test these.
> These tests were a stopgap solution anyway while the reverse builtbot was
> being setup.
>
> > I'll just do that (r310506 & r310508) - done! :)
> Thanks!
>
>
> --Mandeep
>
> On 8/9/2017 11:35 AM, David Blaikie wrote:
>
>
>
> On Wed, Aug 9, 2017 at 1:44 AM Grang, Mandeep Singh <mgrang at codeaurora.org>
> wrote:
>
>> In D35043 I have removed the llvm tests which use -reverse-iterate. This
>> patch removes the clang tests.
>>
>
> Ah, OK. I'm still curious about whether this results in a loss of test
> coverage. Without this test, would the bug it was testing still be caught
> by some test failure in at least one of the forward or reverse iteration
> modes?
>
>
>> Should I post a later patch to change all "class PointerLikeTypeTraits"
>> to "struct PointerLikeTypeTraits"?
>>
>
> I'll just do that (r310506 & r310508) - done! :)
>
>
>>
>>
>> On 8/7/2017 2:50 PM, David Blaikie wrote:
>>
>>
>>
>> On Mon, Aug 7, 2017 at 12:08 PM Mandeep Singh Grang via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> mgrang added a comment.
>>>
>>> This patch does 3 things:
>>>
>>> 1. Get rid of the unit test objc-modern-metadata-visibility2.mm because
>>> this test check uses flag -reverse-iterate. This flag will be removed in
>>> https://reviews.llvm.org/D35043.
>>>
>>
>> Sure - please commit that separately (probably once D35043 is approved -
>> probably best to include that removal in D35043, or a separate patch that
>> /only/ removes the -reverse-iterate flag (& any tests that use it) as a
>> standalone change?).
>>
>> Does this test need a replacement? If this test is removed and the
>> underlying issue it was testing regresses, will one of the buildbots
>> (reverse or normal) catch the problem?
>>
>>
>>> 2. https://reviews.llvm.org/D35043 gets rid of the empty base
>>> definition for PointerLikeTypeTraits. This results in a compiler warning
>>> because PointerLikeTypeTrait has been defined as struct here while in the
>>> header it is a class. So I have changed struct to class.
>>>
>>
>> I'd probably go the other way - traits classes like this make more sense
>> as structs, I think - it only has public members & no implementation really
>> has any need for supporting private members.
>>
>>
>>> 3. Since I changed struct PointerLikeTypeTrait to class
>>> PointerLikeTypeTrait here, the member functions are no longer public now.
>>> This results in a compiler error. So I explicitly marked them as public
>>> here.
>>>
>>>
>>> https://reviews.llvm.org/D36386
>>>
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170809/089d1672/attachment.html>