<div class="gmail_quote">On Mon, Jul 16, 2012 at 8:13 AM, Nikola Smiljanic <span dir="ltr"><<a href="mailto:popizdeh@gmail.com" target="_blank">popizdeh@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I kindly request another review.<div><br></div><div>while (int f()=0) ; // condition.cpp
</div><div>This should be the same as your ConditionContext example so I removed warnings.</div></blockquote><div><br></div><div>Perfect.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>T(*d)(int(p)); // decl-expr-ambiguity.cpp
</div><div>This should be unambiguous, always a function pointer. Maybe I should delete the entire line since this file is explicitly testing ambiguity?</div>
</blockquote></div><br><div>We should still be warning on this line, because it could either be declaring d to be a pointer to a function which returns T, or to be a pointer to T initialized by int(p). The place to handle this is probably GetFullTypeForDeclarator in SemaType.cpp. I don't know if you can move all of this checking to there, but if you can, that would be ideal. If not, splitting it up is OK.</div>
<div><br></div><div><br></div><div><div>+++ include/clang/Sema/DeclSpec.h<span class="Apple-tab-span" style="white-space:pre">        </span>(working copy)</div><div>@@ -1488,6 +1488,9 @@</div><div>   /// \brief Is this Declarator a redeclaration?</div>
<div>   bool Redeclaration : 1;</div><div> </div><div>+  /// \brief Can this declaration be a constructor-style initializer?</div><div>+  bool Ambiguous : 1;</div></div><div><br></div><div>I think this flag really belongs on DeclaratorChunk::FunctionTypeInfo. Sorry for suggesting putting it here!</div>
<div><br></div><div><br></div><div><div>+++ lib/Sema/SemaDecl.cpp<span class="Apple-tab-span" style="white-space:pre">        </span>(working copy)</div><div>@@ -5261,10 +5261,25 @@</div><div>+      if (!T->isVoidType() && C.Fun.NumArgs != 0 && D.isAmbiguous() &&</div>
</div><div>[...]</div><div>+      if (!T->isVoidType() && C.Fun.NumArgs == 0 && D.isAmbiguous()) {</div><div><br></div><div>Please factor the D.isAmbiguous() && !T->isVoidType() tests into an outer 'if'.</div>
<div><br></div><div><br></div><div><div>+++ lib/Parse/ParseTentative.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -671,7 +671,8 @@</div></div><div><div>+      bool Ambiuous = false;</div>
<div>+      if (!mayBeAbstract && !isCXXFunctionDeclarator(Ambiuous))</div></div><div><br></div><div>Typo (missing 'g'). Or even, drop this change, and instead...</div><div><br></div><div><br></div><div><div>
+++ include/clang/Parse/Parser.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -1643,11 +1643,11 @@</div><div>-  bool isCXXFunctionDeclarator(bool warnIfAmbiguous);</div></div><div>
+  bool isCXXFunctionDeclarator(bool &isAmbiguous);</div><div><br></div><div>... use (bool *isAmbiguous = 0). Also, since you're touching this variable anyway, would you mind renaming to IsAmbiguous to conform with the coding standard?</div>
<div><br></div><div><br></div><div><div>+++ include/clang/Basic/DiagnosticSemaKinds.td<span class="Apple-tab-span" style="white-space:pre">   </span>(working copy)</div><div>@@ -158,6 +158,11 @@</div></div><div>+def warn_parens_disambiguated_as_function_decl : Warning<</div>
<div><div>+  "parentheses were disambiguated as a function declarator">,</div><div>+  InGroup<VexingParse>;</div></div><div><br></div><div>It would be great to say "as a function declaration" here (but still use the "as a function declarator" in the pointer-to-function case).</div>
<div><br></div><div><br></div><div>Thanks for working on this!</div><div>Richard</div>