[cfe-commits] [PATCH] PR10053 Improved two-stage lookup diagnostic

[Richard Smith]

Hi Chandler,

I've made the requested changes and checked this in as r132672.

On Sun, June 5, 2011 21:47, Chandler Carruth wrote:
> 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.

Done.

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

Done. I only implemented it this way round to match DiagnoseEmptyLookup.

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

OK.

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

Done. This is not trivial: SmallPtrSet::erase does not document its
iterator invalidation semantics, and the normal approach for sets
(increment the iterator before erasing) crashed.

> +        if (SemaRef.StdNamespace) {
> +          DeclContext *Std = SemaRef.getStdNamespace();
>
>
> maybe:
> if (DeclContext *Std = SemaRef.getStdNamespace()) {
>
> <skip>
>
>
> +}
>
>
> whitespace and a doxygen comment here?

Done.

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

I've put in a FIXME.

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

Same as above.

Thanks,
Richard