[cfe-commits] [PATCH] PR10053 Improved two-stage lookup diagnostic
Chandler Carruth
chandlerc at google.com
Sun Jun 5 13:47:27 PDT 2011
Started to comment on this patch in IRC, but i'll switch to here in case
that gets lost...
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.
The rest of my comments are really just nits or style issues. Nothing
substantial.
On to the patch....
===================================================================
--- lib/Sema/SemaOverload.cpp (revision 132445)
+++ lib/Sema/SemaOverload.cpp (working copy)
@@ -7828,6 +7828,104 @@
ULE->isStdAssociatedNamespace());
}
+/// Attempt to recover from ill-formed use of non-dependent names in
templates
+/// where the non-dependent name was declared after the template was
defined.
+/// This is common in code written for a compilers which do not correctly
+/// implement two-stage name lookup.
+///
+/// Returns false if a viable candidate was found.
Can we return true on success and false on failure to diagnose? That is so
much more intuitive to me.
<skip>
+ OverloadCandidateSet::iterator Best;
+ if (Candidates.BestViableFunction(SemaRef, FnLoc, Best) ==
OR_Success) {
Invert this condition, and return true? Early exit here allows the rest of
this to be less indented.
<skip>
+ // Never suggest declaring a function within namespace 'std'.
+ Sema::AssociatedNamespaceSet SuggestedNamespaces;
Why not remove the enclosed namespaces from the associated set rather than
adding non-enclosed namespaces to a new set?
+ if (SemaRef.StdNamespace) {
+ DeclContext *Std = SemaRef.getStdNamespace();
maybe:
if (DeclContext *Std = SemaRef.getStdNamespace()) {
<skip>
+}
whitespace and a doxygen comment here?
+static bool
+DiagnoseTwoStageOperatorLookup(Sema &SemaRef, OverloadedOperatorKind Op,
<skip>
@@ -8129,6 +8240,12 @@
}
case OR_No_Viable_Function:
+ // This is an erroneous use of an operator which can be overloaded by
+ // a non-member function. Check for non-member operators which were
+ // defined too late to be candidates.
+ if (!DiagnoseTwoStageOperatorLookup(*this, Op, OpLoc, Args, NumArgs))
+ return ExprError();
+
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.
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.
A comment would be good here. Also, should we leave a fixme to eventually
recover? There doesn't seem to be any fundamental obstacle?
@@ -8129,6 +8240,12 @@
}
case OR_No_Viable_Function:
+ // This is an erroneous use of an operator which can be overloaded by
+ // a non-member function. Check for non-member operators which were
+ // defined too late to be candidates.
+ if (!DiagnoseTwoStageOperatorLookup(*this, Op, OpLoc, Args, NumArgs))
+ return ExprError();
+
Same as above...
On Sun, Jun 5, 2011 at 10:31 AM, Richard Smith <richard at metafoo.co.uk>wrote:
> Oops, forgot the patch!
>
> On Sun, June 5, 2011 18:31, Richard Smith wrote:
> > On Sun, June 5, 2011 04:16, Douglas Gregor wrote:
> >> On Jun 4, 2011, at 1:37 PM, Richard Smith wrote:
> >>> The attached patch improves our error recovery when handling code
> >>> which some versions of gcc incorrectly accept, due to an incomplete
> >>> implementation of two-stage name lookup.
> >>>
> >>> This example from the compatibility section on the www:
> >>>
> >>>
> >>>
> >>> template <typename T> T Squared(T x) { return Multiply(x, x); }
> >>>
> >>>
> >>> int Multiply(int x, int y) { return x * y; }
> >>>
> >>>
> >>> int main() { Squared(5); }
> >>>
> >>>
> >>>
> >>> Now produces the following diagnostics:
> >>>
> >>>
> >>>
> >>> my_file.cpp:2:10: error: use of undeclared identifier 'Multiply'
> >>> return Multiply(x, x); ^ my_file.cpp:10:3: note: in instantiation of
> >>> function template specialization 'Squared<int>' requested here
> >>> Squared(5);
> >>> ^
> >>> my_file.cpp:5:5: note: viable function not candidate: declared after
> >>> template was defined and not in an associated namespace int
> >>> Multiply(int
> >>> x, int y) { ^
> >>
> >> The code looks good, but I think the diagnostic wording could be
> >> improved considerably. The error message should say what the problem is,
> >> and the note should suggest the solution. For example:
> >>
> >> error: call to function 'Multiply' that is neither visible in the
> >> template definition nor found by argument dependent lookup
> >>
> >> note: 'Multiply' should be declared prior to the call site or in the
> >> namespace of one of its arguments
> >
> > The attached, revised patch produces this in the case where there are no
> > associated namespaces:
> >
> > my_file.cpp:2:10: error: call to function 'Multiply' that is neither
> > visible in the template definition nor found by argument dependent lookup
> > return Multiply(x, x); ^
> > my_file.cpp:10:3: note: in instantiation of function template
> > specialization 'Squared<int>' requested here Squared(5);
> > ^
> > my_file.cpp:5:5: note: 'Multiply' should be declared prior to the call
> > site int Multiply(int x, int y) { ^
> >
> >
> > In the case where there is a single associated namespace (excluding those
> > within namespace std), we produce this:
> >
> > my_file2.cpp:5:13: error: call to function 'operator<<' that is neither
> > visible in the template definition nor found by argument dependent lookup
> > std::cout << value << "\n";
> > ^
> > my_file2.cpp:17:3: note: in instantiation of function template
> > specialization 'Dump<ns::Data>' requested here Dump(ns::Data());
> > ^
> > my_file2.cpp:12:15: note: 'operator<<' should be declared prior to the
> > call site or in namespace 'ns' std::ostream& operator<<(std::ostream&
> out,
> > ns::Data data) {
> > ^
> >
> >
> > Finally, if there are multiple associated namespaces for the call, we
> > produce a slightly tweaked version of the diagnostic you suggested:
> >
> > my_file3.cpp:5:10: error: call to function 'operator!' that is neither
> > visible in the template definition nor found by argument dependent lookup
> > return !value; ^
> > my_file3.cpp:18:3: note: in instantiation of function template
> > specialization 'Negate<ns2::S<ns::Data> >' requested here
> > Negate(ns2::S<ns::Data>());
> > ^
> > my_file3.cpp:15:18: note: 'operator!' should be declared prior to the
> > call site or in an associated namespace of one of its arguments
> > ns2::S<ns::Data> operator!(ns2::S<ns::Data>);
> > ^
> >
> >
> >>> This patch only diagnoses normal unqualified names; diagnostics for
> >>> names with scope specifiers and for overloaded operators are not
> >>> similarly improved.
> >
> > I've extended the patch to also cover overloaded operators. We don't
> > fully recover in that case, but we do produce the improved diagnostic.
> >
> > Comments?
> >
> >
> > Thanks,
> > Richard
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110605/82aa7b48/attachment.html>
More information about the cfe-commits
mailing list