<div dir="ltr">Hi Dave,<div><br></div><div>100% code coverage sounds like a good aim, but I'm not sure it applies here: As of this patch there's no code in the ELF or COFF parser for this directive. It just goes in the "unsupported directive" bucket. As long as we're testing one unsupported directive (though to be fair I'm not sure we are) we've covered all of the relevant code.</div><div><br></div><div>- Lang.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 11, 2016 at 12:48 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Mon, Apr 11, 2016 at 12:44 PM, Lang Hames via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Rafael,<div><br></div><div>Yep - this was just a temporary hack. It's been fixed as of r<span style="font-size:13px">265975.</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">Regarding </span>test cases - what's the bar for something like this. I'd have included a test that the directive was handled correctly on the platforms it's intended for, but testing every (unsupported) directive on every platform seems like overkill.</div></div></blockquote><div><br></div></span><div>FWIW, my bar would be "if it's separate code, it probably deserves a separate test". (100% block coverage is a pretty low bar for testing (& I expect a bit more than that - it's easy to have coverage without verification of the right behavior, for example), and, admittedly, not one (m)any projects really have, especially with C++ & many implicit blocks (maybe that's not so bad with exceptions disabled), but generally something to strive for)<br><br>- Dave</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span><font color="#888888"><div><br></div><div>- Lang.</div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Apr 10, 2016 at 8:35 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks, but:<br>
<br>
* This should have a testcase.<br>
* It is odd to parse and then issue an error. I think the correct fix<br>
is to move the parsing to lib/MC/MCParser/DarwinAsmParser.cpp, that<br>
way we get the natural error on ELF and COFF.<br>
<br>
Cheers,<br>
Rafael<br>
<br>
<br>
<br>
On 8 April 2016 at 13:38, Lang Hames via llvm-commits<br>
<div><div><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: lhames<br>
> Date: Fri Apr 8 12:38:51 2016<br>
> New Revision: 265815<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=265815&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=265815&view=rev</a><br>
> Log:<br>
> [Object] Report an error if .alt_entry is used with ELF or COFF.<br>
><br>
> I'm looking into a better way to do this long-term, but for now at least don't<br>
> crash.<br>
><br>
> Modified:<br>
> llvm/trunk/lib/MC/MCELFStreamer.cpp<br>
> llvm/trunk/lib/MC/WinCOFFStreamer.cpp<br>
><br>
> Modified: llvm/trunk/lib/MC/MCELFStreamer.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCELFStreamer.cpp?rev=265815&r1=265814&r2=265815&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCELFStreamer.cpp?rev=265815&r1=265814&r2=265815&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/MC/MCELFStreamer.cpp (original)<br>
> +++ llvm/trunk/lib/MC/MCELFStreamer.cpp Fri Apr 8 12:38:51 2016<br>
> @@ -285,7 +285,7 @@ bool MCELFStreamer::EmitSymbolAttribute(<br>
> break;<br>
><br>
> case MCSA_AltEntry:<br>
> - llvm_unreachable("ELF doesn't support this attribute");<br>
> + report_fatal_error("ELF doesn't support the .alt_entry attribute");<br>
> }<br>
><br>
> return true;<br>
><br>
> Modified: llvm/trunk/lib/MC/WinCOFFStreamer.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/WinCOFFStreamer.cpp?rev=265815&r1=265814&r2=265815&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/WinCOFFStreamer.cpp?rev=265815&r1=265814&r2=265815&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/MC/WinCOFFStreamer.cpp (original)<br>
> +++ llvm/trunk/lib/MC/WinCOFFStreamer.cpp Fri Apr 8 12:38:51 2016<br>
> @@ -107,6 +107,8 @@ bool MCWinCOFFStreamer::EmitSymbolAttrib<br>
> case MCSA_Global:<br>
> Symbol->setExternal(true);<br>
> break;<br>
> + case MCSA_AltEntry:<br>
> + report_fatal_error("COFF doesn't support the .alt_entry attribute");<br>
> }<br>
><br>
> return true;<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>