<div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 24, 2012 at 2:37 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Tue, Apr 24, 2012 at 2:28 PM, Kaelyn Uhrain <<a href="mailto:rikka@google.com">rikka@google.com</a>> wrote:<br>
> This patch adds a suggestion and related fixit for when a member reference<br>
> using '.' such as "foo.bar()" fails because bar() is not a member of foo,<br>
> but foo defines an operator-> and bar() is a member of the object returned<br>
> by foo's operator->.<br>
<br>
</div>Oh, nice - I'd noticed this was lacking a while ago, glad to see it<br>
being improved/fixed.<br>
<div class="im"><br>
> A more concrete case where this is helpful is with<br>
> classes like std::shared_ptr or even clang's QualType that have an<br>
> operator-> and the user accidentally types "." where they meant to use "->"<br>
> (since, you know, the variables in question aren't actually pointers).<br>
><br>
> I'd like some pre-commit feedback since:<br>
>   * while it's a fairly simple patch, there is some ugliness to it for<br>
> trying out the correction to see if it causes additional errors,<br>
<br>
</div>I don't have a very informed opinion here - but for what it's worth I<br>
think the code is fairly simple/tidy/isolated, given what it's doing.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
>   * the error text feels a bit verbose, though it's hardly the longest error<br>
> message and I can't think of a good way to shorten it, and<br>
<br>
</div>The text seems reasonable, though it's a bit long in your example in<br>
part because of the names - but also because we're printing the fully<br>
qualified name of the base expression's type. Is there any<br>
reasonable/easy way we can print something closer to/exactly as the<br>
user wrote? ("wrapped_ptr<Worker>") - I'm not sure whether that's a<br>
good thing to do, or how it compares to other diagnostics that might<br>
have this problem.<br></blockquote><div><br></div><div>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. ;)</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
>   * I am undecided whether there should be an accompanying note, and if so<br>
> what the note should refer to.<br>
<br>
</div>I doubt it - but it shouldn't be too hard to compare this diagnostic<br>
to the other ./-> mismatch recoveries we have to ensure consistency &<br>
I think that's probably sufficient.<br></blockquote><div><br></div><div>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.</div>
<div> </div><div>If there are no objections, I shall commit the patch as-is.</div><div><br></div><div>Thanks,</div><div>Kaelyn</div></div></div>