<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Nov 27, 2014 at 2:50 PM, Vassil Vassilev <span dir="ltr"><<a href="mailto:vvasilev@cern.ch" target="_blank">vvasilev@cern.ch</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span class="">
<div>On 25/11/14 00:52, Richard Smith wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><span>+
}
+ else if
(!isUnresolvedExceptionSpec(FPT->getExceptionSpecType())
&&</span>
<div><span><br>
</span></div>
Per the LLVM coding style, put the close brace and the 'else' on
the same line.
<div><br>
<div><span>+
isUnresolvedExceptionSpec(PrevFPT->getExceptionSpecType()))
{
+ // Happens in cases where module A contains only a fwd
decl and module B
+ // contains the definition.</span></div>
<div><span><br>
</span></div>
There are other conditions here; I don't think this comment is
particularly helpful. Just remove it?</div>
<div><br>
<div><span>+
FunctionProtoType::ExtProtoInfo EPI =
FPT->getExtProtoInfo();
+ while (PrevFD) {
+ Reader.Context.adjustExceptionSpec(PrevFD,
EPI.ExceptionSpec);
+ PrevFD = PrevFD->getPreviousDecl();</span></div>
<div><span><br>
</span></div>
Please use a separate variable for this loop rather than
reusing PrevFD. It's only by coincidence that this code is at
the end of the function; it shouldn't be changing
function-scope state like this.</div>
</div>
</blockquote></span>
All good points, thanks!<span class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>It seems like it should be possible to produce a testcase
for this. You'd need something like:</div>
<div><br>
</div>
<div>A.h:</div>
<div><br>
</div>
<div>struct A { A(); } b{}, c(b); // computed exception spec
for copy ctor and move ctor<br>
</div>
<div><br>
</div>
<div>B.h:</div>
<div><br>
</div>
<div>
<div>struct A { A(); } a{}; // EST_Unevaluated for copy ctor
and move ctor</div>
<div><br>
</div>
</div>
<div>... then import A and B, and do something that assumes that
every declaration has an exception specification if any
declaration does.</div>
</div>
</blockquote></span>
Thanks for the pointer. I managed to reproduce the behavior, i.e
unevaluated exception spec as a first/canonical redecl (see the new
attached patch). However this test doesn't trigger the original
issue (and thus not testing anything :( )
<a href="http://llvm.org/bugs/show_bug.cgi?id=21687" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=21687</a><br>
There the setup is more difficult, because I need to generate a
unevaluated exception spec dtor as a first redecl and go through
clang::FunctionProtoType::isNothrow to get the assertion. To make
things worse, this base dtor must be emitted as an alias. Could you
help me out?<br></div></blockquote><div><br></div><div>Ugh, getting good modules testcases is hard. I'm not sure I follow what you said above, though: does the testcase in your patch fail without the code change and pass with it? If so, I think that's enough. (Or does it only fail with a previous revision of the patch and pass with both trunk and the patched code?)</div><div><br></div><div><span style="color:rgb(0,0,0);font-family:'Courier New',Courier,monospace;font-size:14px;white-space:pre-wrap">+ FunctionDecl* PrevFDToUpdate = PrevFD;
+ while (PrevFDToUpdate) {
+ Reader.Context.adjustExceptionSpec(PrevFDToUpdate, EPI.ExceptionSpec);
+ PrevFDToUpdate = PrevFDToUpdate->getPreviousDecl();
</span></div><div><span style="color:rgb(0,0,0);font-family:'Courier New',Courier,monospace;font-size:14px;white-space:pre-wrap"><br></span></div><div>The "*" should go on the right, and this would (to me) read better as a for loop:<br></div><div><br></div><div>for (FunctionDecl *PrevFDToUpdate = PrevFD; PrevFDToUpdate;</div><div> PrevFDToUpdate = PrevFDToUpdate->getPreviousDecl())</div><div> Reader.Context.adjustExceptionSpec(PrevFDToUpdate, EPI.ExceptionSpec);</div><div><br></div><div>Other than that, LGTM.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
Vassil<span class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Mon, Nov 24, 2014 at 11:09 AM,
Vassil Vassilev <span dir="ltr"><<a href="mailto:vvasilev@cern.ch" target="_blank">vvasilev@cern.ch</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>Sorry for the delay. Attaching the new version.<span><font color="#888888"><br>
Vassil</font></span>
<div>
<div><br>
On 14/11/14 02:47, Richard Smith wrote:<br>
</div>
</div>
</div>
<div>
<div>
<blockquote type="cite">
<div dir="ltr">
<div>+ }</div>
<div>+ else { //
FPT->getExceptionSpecType() is resolved
and the other is not</div>
<div><br>
</div>
<div>You're not checking for this condition;
the code here is getting run even if both
or neither are unresolved.</div>
<div><br>
</div>
<div>The patch needs rebasing (we have a new
helper function in ASTContext to update
the exception specification of a
declaration), but looks like the right
direction.</div>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Thu, Nov 6, 2014
at 4:23 AM, Vassil Vassilev <span dir="ltr"><<a href="mailto:vasil.georgiev.vasilev@cern.ch" target="_blank">vasil.georgiev.vasilev@cern.ch</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi
Richard,<br>
I am attaching the patch we discussed
at the dev meeting. Still haven't found
small reproducer...<br>
The issue appears to stem from the
fact that module A contains only a
forward declaration of a function and it
exception spec cannot be computed. In
module B is the definition with computed
exception spec, which gets deserialized
after the one in module A. This patch
teaches the ASTDeclReader to update all
the exception specs of the previous
decls to the current one.<br>
<br>
Could you review, please?<br>
Many thanks,<br>
Vassil</blockquote>
</div>
</div>
</blockquote>
<br>
</div>
</div>
</div>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
<br>
</span><span class=""><font color="#888888"><pre cols="72">--
--------------------------------------------
Q: Why is this email five sentences or less?
A: <a href="http://five.sentenc.es" target="_blank">http://five.sentenc.es</a>
</pre>
</font></span></div>
</blockquote></div><br></div></div>