Oops, that was meant to be a reply-all, sorry. On Wed, Apr 25, 2012 at 12:47 PM, David Blaikie <[email protected]> wrote: > On Wed, Apr 25, 2012 at 11:19 AM, Kaelyn Uhrain <[email protected]> wrote: >> On Tue, Apr 24, 2012 at 2:37 PM, David Blaikie <[email protected]> wrote: >>> >>> On Tue, Apr 24, 2012 at 2:28 PM, Kaelyn Uhrain <[email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
