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