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

[Grang, Mandeep Singh via cfe-commits]

 > 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 <mailto: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
>>     <mailto: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
>>         <http://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/cfe-commits/attachments/20170809/e3e16b88/attachment.html>