[cfe-commits] [PATCH] Add suggestion for replacing '.' with '->' in failed member reference

[Kaelyn Uhrain]

On Tue, Apr 24, 2012 at 2:37 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Apr 24, 2012 at 2:28 PM, Kaelyn Uhrain <rikka at google.com> wrote:
> > This patch adds a suggestion and related fixit for when a member
> reference
> > using '.' such as "foo.bar()" fails because bar() is not a member of foo,
> > but foo defines an operator-> and bar() is a member of the object
> returned
> > by foo's operator->.
>
> Oh, nice - I'd noticed this was lacking a while ago, glad to see it
> being improved/fixed.
>
> > A more concrete case where this is helpful is with
> > classes like std::shared_ptr or even clang's QualType that have an
> > operator-> and the user accidentally types "." where they meant to use
> "->"
> > (since, you know, the variables in question aren't actually pointers).
> >
> > I'd like some pre-commit feedback since:
> >   * while it's a fairly simple patch, there is some ugliness to it for
> > trying out the correction to see if it causes additional errors,
>
> I don't have a very informed opinion here - but for what it's worth I
> think the code is fairly simple/tidy/isolated, given what it's doing.
>

That makes me feel a bit better. :) The main piece I felt was kind of ugly
was the optional struct passed to BuildMemberReferenceExpr so it can have
the remaining values needed call ActOnMemberAccess with the fixed-up
expression.


> >   * the error text feels a bit verbose, though it's hardly the longest
> error
> > message and I can't think of a good way to shorten it, and
>
> The text seems reasonable, though it's a bit long in your example in
> part because of the names - but also because we're printing the fully
> qualified name of the base expression's type. Is there any
> reasonable/easy way we can print something closer to/exactly as the
> user wrote? ("wrapped_ptr<Worker>") - I'm not sure whether that's a
> good thing to do, or how it compares to other diagnostics that might
> have this problem.
>

It's apparently how the diagnostics printer renders a DeclContext. While
it's kind of ugly in the trivial situation of the unit test, I think we
probably want to keep it to avoid name (and namespace) confusion in more
complex examples. Particularly when one of the classes is pulled in from a
header file. ;)


> >   * I am undecided whether there should be an accompanying note, and if
> so
> > what the note should refer to.
>
> I doubt it - but it shouldn't be too hard to compare this diagnostic
> to the other ./-> mismatch recoveries we have to ensure consistency &
> I think that's probably sufficient.
>

There is only one other message in DiagnosticSemaKinds.td that suggests an
"->": err_typecheck_member_reference_suggestion gives the suggestion as
"maybe you meant to use '->'?" which seems too weak of a wording in this
case. Since this patch is a typo correction with a fixit hint, I modeled
the text after many of the typo suggestion messages I've encountered.

If there are no objections, I shall commit the patch as-is.

Thanks,
Kaelyn
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120425/a15a57ca/attachment.html>