<div dir="ltr">OK - go ahead and remove the tests then, if the functionality is now covered by the buildbot+existing tests.</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 9, 2017 at 12:04 PM Grang, Mandeep Singh <<a href="mailto:mgrang@codeaurora.org">mgrang@codeaurora.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
<p>> 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?</p>
</div><div text="#000000" bgcolor="#FFFFFF"><p>Sorry ... I missed that. Yes, the reverse iteration buildbot <span><a href="http://lab.llvm.org:8011/builders/reverse-iteration" target="_blank">http://lab.llvm.org:8011/builders/reverse-iteration</a>
</span>would test these. These tests were a stopgap solution
anyway while the reverse builtbot was being setup.</p></div><div text="#000000" bgcolor="#FFFFFF">
<br>
> I'll just do that (r310506 & r310508) - done! :)<br></div><div text="#000000" bgcolor="#FFFFFF">
Thanks!</div><div text="#000000" bgcolor="#FFFFFF"><br>
<p><span><br>
</span></p>
<p><span>--Mandeep<br>
</span></p></div><div text="#000000" bgcolor="#FFFFFF">
<br>
<div class="m_-2255342075703020088moz-cite-prefix">On 8/9/2017 11:35 AM, David Blaikie
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Aug 9, 2017 at 1:44 AM Grang, Mandeep
Singh <<a href="mailto:mgrang@codeaurora.org" target="_blank">mgrang@codeaurora.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div> In D35043 I have removed the llvm tests which use
-reverse-iterate. This patch removes the clang tests.<br>
</div>
</blockquote>
<div><br>
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?<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>Should I post a later patch to change all "class
PointerLikeTypeTraits" to "struct PointerLikeTypeTraits"?</div>
</blockquote>
<div><br>
I'll just do that (r310506 & r310508) - done! :)<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
<br>
<div class="m_-2255342075703020088m_-544102029957928977moz-cite-prefix">On
8/7/2017 2:50 PM, David Blaikie wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, Aug 7, 2017 at 12:08 PM
Mandeep Singh Grang via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">mgrang
added a comment.<br>
<br>
This patch does 3 things:<br>
<br>
1. Get rid of the unit test <a href="http://objc-modern-metadata-visibility2.mm" rel="noreferrer" target="_blank">objc-modern-metadata-visibility2.mm</a>
because this test check uses flag
-reverse-iterate. This flag will be removed in <a href="https://reviews.llvm.org/D35043" rel="noreferrer" target="_blank">https://reviews.llvm.org/D35043</a>.<br>
</blockquote>
<div><br>
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?).<br>
<br>
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?<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">2.
<a href="https://reviews.llvm.org/D35043" rel="noreferrer" target="_blank">https://reviews.llvm.org/D35043</a>
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.<br>
</blockquote>
<div><br>
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.<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D36386" rel="noreferrer" target="_blank">https://reviews.llvm.org/D36386</a><br>
<br>
<br>
<br>
</blockquote>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
</div>
</blockquote>
<br>
</div></blockquote></div>