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. > > * 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
