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

[David Blaikie]

Oops, that was meant to be a reply-all, sorry.

On Wed, Apr 25, 2012 at 12:47 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Wed, Apr 25, 2012 at 11:19 AM, Kaelyn Uhrain <rikka at google.com> wrote:
>> 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.
>
> Yeah - that seems fairly unavoidable & puts the right data in the
> right places for making the relevant decisions.
>
>>> >   * 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. ;)
>
> Yep, perhaps - I was just wondering that the precedent on other type
> printing diagnostics was. If we don't usually print the unqualified
> user-typed name, that's fine.
>
>>> >   * 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.
>
> Yeah, that's a little terse - easier when you used '.' on a
> pointer-to-struct since you can't mean '.' there at all, whereas here
> it's a bit more subtle.
>
>> 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.
>
> Sounds good.
>
> [+lhames because I think he asked me about this missing diagnostic
> when he was dealing with iterators a few months ago]
>
> - David