[modules][PATCH] Teach the ASTDeclReader to update the exception specs of the deserialized decl's redecl chain.

[Richard Smith]

On Thu, Nov 27, 2014 at 2:50 PM, Vassil Vassilev <vvasilev at cern.ch> wrote:

>  On 25/11/14 00:52, Richard Smith wrote:
>
> + } + else if (!isUnresolvedExceptionSpec(FPT->getExceptionSpecType()) &&
>
>  Per the LLVM coding style, put the close brace and the 'else' on the same
> line.
>
> + isUnresolvedExceptionSpec(PrevFPT->getExceptionSpecType())) { + //
> Happens in cases where module A contains only a fwd decl and module B + //
> contains the definition.
>
>  There are other conditions here; I don't think this comment is
> particularly helpful. Just remove it?
>
> + FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo(); + while
> (PrevFD) { + Reader.Context.adjustExceptionSpec(PrevFD, EPI.ExceptionSpec);
> + PrevFD = PrevFD->getPreviousDecl();
>
>  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.
>
> All good points, thanks!
>
>
>  It seems like it should be possible to produce a testcase for this.
> You'd need something like:
>
>  A.h:
>
>  struct A { A(); } b{}, c(b);  // computed exception spec for copy ctor
> and move ctor
>
>  B.h:
>
>  struct A { A(); } a{};  // EST_Unevaluated for copy ctor and move ctor
>
>  ... then import A and B, and do something that assumes that every
> declaration has an exception specification if any declaration does.
>
> 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 :( ) http://llvm.org/bugs/show_bug.cgi?id=21687
> 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?
>

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?)

+ FunctionDecl* PrevFDToUpdate = PrevFD; + while (PrevFDToUpdate) { +
Reader.Context.adjustExceptionSpec(PrevFDToUpdate, EPI.ExceptionSpec); +
PrevFDToUpdate = PrevFDToUpdate->getPreviousDecl();

The "*" should go on the right, and this would (to me) read better as a for
loop:

for (FunctionDecl *PrevFDToUpdate = PrevFD; PrevFDToUpdate;
     PrevFDToUpdate = PrevFDToUpdate->getPreviousDecl())
  Reader.Context.adjustExceptionSpec(PrevFDToUpdate, EPI.ExceptionSpec);

Other than that, LGTM.


> Vassil
>
>
> On Mon, Nov 24, 2014 at 11:09 AM, Vassil Vassilev <vvasilev at cern.ch>
> wrote:
>
>>  Sorry for the delay. Attaching the new version.
>> Vassil
>>
>> On 14/11/14 02:47, Richard Smith wrote:
>>
>>  +    }
>> +    else { // FPT->getExceptionSpecType() is resolved and the other is
>> not
>>
>>  You're not checking for this condition; the code here is getting run
>> even if both or neither are unresolved.
>>
>>  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.
>>
>> On Thu, Nov 6, 2014 at 4:23 AM, Vassil Vassilev <
>> vasil.georgiev.vasilev at cern.ch> wrote:
>>
>>> Hi Richard,
>>>   I am attaching the patch we discussed at the dev meeting. Still
>>> haven't found small reproducer...
>>>   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.
>>>
>>>   Could you review, please?
>>> Many thanks,
>>> Vassil
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
>
> --
> --------------------------------------------
> Q: Why is this email five sentences or less?
> A: http://five.sentenc.es
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150113/8a1a2447/attachment.html>