Started to comment on this patch in IRC, but i'll switch to here in case that gets lost...<div><br></div><div>First comment from my perspective is that I'd rather consistently call this "two-phase" rather than "two-stage" lookup. The former is what dgregor's blog post uses, etc.</div>
<div><br></div><div>The rest of my comments are really just nits or style issues. Nothing substantial.</div><div><br></div><div>On to the patch....</div><div><br></div><div><div>===================================================================</div>
<div>--- lib/Sema/SemaOverload.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 132445)</div><div>+++ lib/Sema/SemaOverload.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div>
<div>@@ -7828,6 +7828,104 @@</div><div> ULE->isStdAssociatedNamespace());</div><div> }</div><div> </div><div>+/// Attempt to recover from ill-formed use of non-dependent names in templates</div>
<div>+/// where the non-dependent name was declared after the template was defined.</div><div>+/// This is common in code written for a compilers which do not correctly</div><div>+/// implement two-stage name lookup.</div>
<div>+///</div><div>+/// Returns false if a viable candidate was found.</div><div><br></div><div>Can we return true on success and false on failure to diagnose? That is so much more intuitive to me.</div><div><br></div><div>
<skip></div><div><br></div><div><div>+ OverloadCandidateSet::iterator Best;</div><div>+ if (Candidates.BestViableFunction(SemaRef, FnLoc, Best) == OR_Success) {</div><div><br></div><div>Invert this condition, and return true? Early exit here allows the rest of this to be less indented.</div>
<div><br></div><div><skip></div><div><br></div><div>+ // Never suggest declaring a function within namespace 'std'. </div></div><div><div>+ Sema::AssociatedNamespaceSet SuggestedNamespaces;</div>
<div><br></div><div>Why not remove the enclosed namespaces from the associated set rather than adding non-enclosed namespaces to a new set?</div><div><br></div><div>+ if (SemaRef.StdNamespace) {</div><div>+ DeclContext *Std = SemaRef.getStdNamespace();</div>
</div><div><br></div><div>maybe:</div><div> if (DeclContext *Std = SemaRef.getStdNamespace()) {</div><div><br></div><div><skip></div><div><br></div><div>+}</div><div><br></div><div>whitespace and a doxygen comment here?</div>
<div><br></div><div>+static bool</div><div>+DiagnoseTwoStageOperatorLookup(Sema &SemaRef, OverloadedOperatorKind Op,</div><div><br></div><div><skip></div><div><br></div><div><div>@@ -8129,6 +8240,12 @@</div><div>
}</div><div> </div><div> case OR_No_Viable_Function:</div><div>+ // This is an erroneous use of an operator which can be overloaded by</div><div>+ // a non-member function. Check for non-member operators which were</div>
<div>+ // defined too late to be candidates.</div><div>+ if (!DiagnoseTwoStageOperatorLookup(*this, Op, OpLoc, Args, NumArgs))</div><div>+ return ExprError();</div><div>+</div></div><div><br></div><div>You return ExprError() here if we return false, but false means successful recovery for this function, so why ExprError()? Above we fall through to ExprError() on true.</div>
<div><br></div><div>Ah this is because the operator variant doesn't recover. So we return the error here as it has at least been diagnosed at this point.</div><div><br></div><div>A comment would be good here. Also, should we leave a fixme to eventually recover? There doesn't seem to be any fundamental obstacle?</div>
<div><br></div><div><div>@@ -8129,6 +8240,12 @@</div><div> }</div><div> </div><div> case OR_No_Viable_Function:</div><div>+ // This is an erroneous use of an operator which can be overloaded by</div><div>+ // a non-member function. Check for non-member operators which were</div>
<div>+ // defined too late to be candidates.</div><div>+ if (!DiagnoseTwoStageOperatorLookup(*this, Op, OpLoc, Args, NumArgs))</div><div>+ return ExprError();</div><div>+</div></div><div><br></div><div>Same as above...</div>
<div><br></div><div><br></div><div class="gmail_quote">On Sun, Jun 5, 2011 at 10:31 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Oops, forgot the patch!<br>
<div><div></div><div class="h5"><br>
On Sun, June 5, 2011 18:31, Richard Smith wrote:<br>
> On Sun, June 5, 2011 04:16, Douglas Gregor wrote:<br>
>> On Jun 4, 2011, at 1:37 PM, Richard Smith wrote:<br>
>>> The attached patch improves our error recovery when handling code<br>
>>> which some versions of gcc incorrectly accept, due to an incomplete<br>
>>> implementation of two-stage name lookup.<br>
>>><br>
>>> This example from the compatibility section on the www:<br>
>>><br>
>>><br>
>>><br>
>>> template <typename T> T Squared(T x) { return Multiply(x, x); }<br>
>>><br>
>>><br>
>>> int Multiply(int x, int y) { return x * y; }<br>
>>><br>
>>><br>
>>> int main() { Squared(5); }<br>
>>><br>
>>><br>
>>><br>
>>> Now produces the following diagnostics:<br>
>>><br>
>>><br>
>>><br>
>>> my_file.cpp:2:10: error: use of undeclared identifier 'Multiply'<br>
>>> return Multiply(x, x); ^ my_file.cpp:10:3: note: in instantiation of<br>
>>> function template specialization 'Squared<int>' requested here<br>
>>> Squared(5);<br>
>>> ^<br>
>>> my_file.cpp:5:5: note: viable function not candidate: declared after<br>
>>> template was defined and not in an associated namespace int<br>
>>> Multiply(int<br>
>>> x, int y) { ^<br>
>><br>
>> The code looks good, but I think the diagnostic wording could be<br>
>> improved considerably. The error message should say what the problem is,<br>
>> and the note should suggest the solution. For example:<br>
>><br>
>> error: call to function 'Multiply' that is neither visible in the<br>
>> template definition nor found by argument dependent lookup<br>
>><br>
>> note: 'Multiply' should be declared prior to the call site or in the<br>
>> namespace of one of its arguments<br>
><br>
> The attached, revised patch produces this in the case where there are no<br>
> associated namespaces:<br>
><br>
> my_file.cpp:2:10: error: call to function 'Multiply' that is neither<br>
> visible in the template definition nor found by argument dependent lookup<br>
> return Multiply(x, x); ^<br>
> my_file.cpp:10:3: note: in instantiation of function template<br>
> specialization 'Squared<int>' requested here Squared(5);<br>
> ^<br>
> my_file.cpp:5:5: note: 'Multiply' should be declared prior to the call<br>
> site int Multiply(int x, int y) { ^<br>
><br>
><br>
> In the case where there is a single associated namespace (excluding those<br>
> within namespace std), we produce this:<br>
><br>
> my_file2.cpp:5:13: error: call to function 'operator<<' that is neither<br>
> visible in the template definition nor found by argument dependent lookup<br>
> std::cout << value << "\n";<br>
> ^<br>
> my_file2.cpp:17:3: note: in instantiation of function template<br>
> specialization 'Dump<ns::Data>' requested here Dump(ns::Data());<br>
> ^<br>
> my_file2.cpp:12:15: note: 'operator<<' should be declared prior to the<br>
> call site or in namespace 'ns' std::ostream& operator<<(std::ostream& out,<br>
> ns::Data data) {<br>
> ^<br>
><br>
><br>
> Finally, if there are multiple associated namespaces for the call, we<br>
> produce a slightly tweaked version of the diagnostic you suggested:<br>
><br>
> my_file3.cpp:5:10: error: call to function 'operator!' that is neither<br>
> visible in the template definition nor found by argument dependent lookup<br>
> return !value; ^<br>
> my_file3.cpp:18:3: note: in instantiation of function template<br>
> specialization 'Negate<ns2::S<ns::Data> >' requested here<br>
> Negate(ns2::S<ns::Data>());<br>
> ^<br>
> my_file3.cpp:15:18: note: 'operator!' should be declared prior to the<br>
> call site or in an associated namespace of one of its arguments<br>
> ns2::S<ns::Data> operator!(ns2::S<ns::Data>);<br>
> ^<br>
><br>
><br>
>>> This patch only diagnoses normal unqualified names; diagnostics for<br>
>>> names with scope specifiers and for overloaded operators are not<br>
>>> similarly improved.<br>
><br>
> I've extended the patch to also cover overloaded operators. We don't<br>
> fully recover in that case, but we do produce the improved diagnostic.<br>
><br>
> Comments?<br>
><br>
><br>
> Thanks,<br>
> Richard<br>
</div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">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>