<div dir="ltr">On Wed, Jul 31, 2013 at 11:00 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=dblaikie@gmail.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">dblaikie@gmail.com</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">
<div class="h5">On Wed, Jul 31, 2013 at 10:38 AM, Kaelyn Uhrain <<a href="mailto:rikka@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>> wrote:<br>
> Author: rikka<br>
> Date: Wed Jul 31 12:38:24 2013<br>
> New Revision: 187504<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=187504&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=187504&view=rev</a><br>
> Log:<br>
> Improve the diagnostic experience, including adding recovery, for<br>
> changing '->' to '.' when there is no operator-> defined for a class.<br>
><br>
> Modified:<br>
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
> cfe/trunk/include/clang/Sema/Sema.h<br>
> cfe/trunk/lib/Sema/SemaExprCXX.cpp<br>
> cfe/trunk/lib/Sema/SemaExprMember.cpp<br>
> cfe/trunk/lib/Sema/SemaOverload.cpp<br>
> cfe/trunk/test/SemaCXX/member-expr.cpp<br>
><br>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=187504&r1=187503&r2=187504&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=187504&r1=187503&r2=187504&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Jul 31 12:38:24 2013<br>
> @@ -4233,6 +4233,8 @@ def err_typecheck_member_reference_sugge<br>
> "member reference type %0 is %select{a|not a}1 pointer; maybe you meant to use '%select{->|.}1'?">;<br>
> def note_typecheck_member_reference_suggestion : Note<<br>
> "did you mean to use '.' instead?">;<br>
> +def note_member_reference_arrow_from_operator_arrow : Note<<br>
> + "'->' applied to return value of the operator->() declared here">;<br>
> def err_typecheck_member_reference_type : Error<<br>
> "cannot refer to type member %0 in %1 with '%select{.|->}2'">;<br>
> def err_typecheck_member_reference_unknown : Error<<br>
><br>
> Modified: cfe/trunk/include/clang/Sema/Sema.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=187504&r1=187503&r2=187504&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=187504&r1=187503&r2=187504&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Sema/Sema.h (original)<br>
> +++ cfe/trunk/include/clang/Sema/Sema.h Wed Jul 31 12:38:24 2013<br>
> @@ -2256,7 +2256,8 @@ public:<br>
> SourceLocation RParenLoc);<br>
><br>
> ExprResult BuildOverloadedArrowExpr(Scope *S, Expr *Base,<br>
> - SourceLocation OpLoc);<br>
> + SourceLocation OpLoc,<br>
> + bool *NoArrowOperatorFound = 0);<br>
><br>
> /// CheckCallReturnType - Checks that a call expression's return type is<br>
> /// complete. Returns true on failure. The location passed in is the location<br>
><br>
> Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=187504&r1=187503&r2=187504&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=187504&r1=187503&r2=187504&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)<br>
> +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Wed Jul 31 12:38:24 2013<br>
> @@ -5076,15 +5076,44 @@ Sema::ActOnStartCXXMemberReference(Scope<br>
> // [...] When operator->returns, the operator-> is applied to the value<br>
> // returned, with the original second operand.<br>
> if (OpKind == tok::arrow) {<br>
> + bool NoArrowOperatorFound = false;<br>
> + bool FirstIteration = true;<br>
> + FunctionDecl *CurFD = dyn_cast<FunctionDecl>(CurContext);<br>
> // The set of types we've considered so far.<br>
> llvm::SmallPtrSet<CanQualType,8> CTypes;<br>
> SmallVector<SourceLocation, 8> Locations;<br>
> CTypes.insert(Context.getCanonicalType(BaseType));<br>
><br>
> while (BaseType->isRecordType()) {<br>
> - Result = BuildOverloadedArrowExpr(S, Base, OpLoc);<br>
> - if (Result.isInvalid())<br>
> + Result = BuildOverloadedArrowExpr(<br>
> + S, Base, OpLoc,<br>
> + // When in a template specialization and on the first loop iteration,<br>
> + // potentially give the default diagnostic (with the fixit in a<br>
> + // separate note) instead of having the error reported back to here<br>
> + // and giving a diagnostic with a fixit attached to the error itself.<br>
> + (FirstIteration && CurFD && CurFD->isFunctionTemplateSpecialization())<br>
> + ? 0<br>
> + : &NoArrowOperatorFound);<br>
> + if (Result.isInvalid()) {<br>
> + if (NoArrowOperatorFound) {<br>
> + if (FirstIteration) {<br>
> + Diag(OpLoc, diag::err_typecheck_member_reference_suggestion)<br>
> + << BaseType << 1 << Base->getSourceRange()<br>
> + << FixItHint::CreateReplacement(OpLoc, ".");<br>
> + OpKind = tok::period;<br>
> + break;<br>
> + } else {<br>
<br>
</div></div>'else' after 'break', just drop the 'else' here.<br></blockquote><div><br></div><div>Done. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5"><br>
> + Diag(OpLoc, diag::err_typecheck_member_reference_arrow)<br>
> + << BaseType << Base->getSourceRange();<br>
> + CallExpr *CE = dyn_cast<CallExpr>(Base);<br>
> + if (Decl *CD = (CE ? CE->getCalleeDecl() : 0)) {<br>
> + Diag(CD->getLocStart(),<br>
> + diag::note_member_reference_arrow_from_operator_arrow);<br>
> + }<br>
> + }<br>
> + }<br>
> return ExprError();<br>
> + }<br>
> Base = Result.get();<br>
> if (CXXOperatorCallExpr *OpCall = dyn_cast<CXXOperatorCallExpr>(Base))<br>
> Locations.push_back(OpCall->getDirectCallee()->getLocation());<br>
> @@ -5096,9 +5125,11 @@ Sema::ActOnStartCXXMemberReference(Scope<br>
> Diag(Locations[i], diag::note_declared_at);<br>
> return ExprError();<br>
> }<br>
> + FirstIteration = false;<br>
> }<br>
><br>
> - if (BaseType->isPointerType() || BaseType->isObjCObjectPointerType())<br>
> + if (OpKind == tok::arrow &&<br>
> + (BaseType->isPointerType() || BaseType->isObjCObjectPointerType()))<br>
> BaseType = BaseType->getPointeeType();<br>
> }<br>
><br>
><br>
> Modified: cfe/trunk/lib/Sema/SemaExprMember.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprMember.cpp?rev=187504&r1=187503&r2=187504&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprMember.cpp?rev=187504&r1=187503&r2=187504&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Sema/SemaExprMember.cpp (original)<br>
> +++ cfe/trunk/lib/Sema/SemaExprMember.cpp Wed Jul 31 12:38:24 2013<br>
> @@ -1136,10 +1136,8 @@ Sema::LookupMemberExpr(LookupResult &R,<br>
> // foo->bar<br>
> // This is actually well-formed in C++ if MyRecord has an<br>
> // overloaded operator->, but that should have been dealt with<br>
> - // by now.<br>
> - Diag(OpLoc, diag::err_typecheck_member_reference_suggestion)<br>
> - << BaseType << int(IsArrow) << BaseExpr.get()->getSourceRange()<br>
> - << FixItHint::CreateReplacement(OpLoc, ".");<br>
> + // by now--or a diagnostic message already issued if a problem<br>
> + // was encountered while looking for the overloaded operator->.<br>
> IsArrow = false;<br>
> } else if (BaseType->isFunctionType()) {<br>
> goto fail;<br>
><br>
> Modified: cfe/trunk/lib/Sema/SemaOverload.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=187504&r1=187503&r2=187504&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=187504&r1=187503&r2=187504&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Sema/SemaOverload.cpp (original)<br>
> +++ cfe/trunk/lib/Sema/SemaOverload.cpp Wed Jul 31 12:38:24 2013<br>
> @@ -11463,7 +11463,8 @@ Sema::BuildCallToObjectOfClassType(Scope<br>
> /// (if one exists), where @c Base is an expression of class type and<br>
> /// @c Member is the name of the member we're trying to find.<br>
> ExprResult<br>
> -Sema::BuildOverloadedArrowExpr(Scope *S, Expr *Base, SourceLocation OpLoc) {<br>
> +Sema::BuildOverloadedArrowExpr(Scope *S, Expr *Base, SourceLocation OpLoc,<br>
> + bool *NoArrowOperatorFound) {<br>
> assert(Base->getType()->isRecordType() &&<br>
> "left-hand side must have class type");<br>
><br>
> @@ -11509,6 +11510,12 @@ Sema::BuildOverloadedArrowExpr(Scope *S,<br>
> case OR_No_Viable_Function:<br>
> if (CandidateSet.empty()) {<br>
> QualType BaseType = Base->getType();<br>
> + if (NoArrowOperatorFound) {<br>
> + // Report this specific error to the caller instead of emitting a<br>
> + // diagnostic, as requested.<br>
> + *NoArrowOperatorFound = true;<br>
> + return ExprError();<br>
> + }<br>
> Diag(OpLoc, diag::err_typecheck_member_reference_arrow)<br>
> << BaseType << Base->getSourceRange();<br>
> if (BaseType->isRecordType() && !BaseType->isPointerType()) {<br>
><br>
> Modified: cfe/trunk/test/SemaCXX/member-expr.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/member-expr.cpp?rev=187504&r1=187503&r2=187504&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/member-expr.cpp?rev=187504&r1=187503&r2=187504&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/SemaCXX/member-expr.cpp (original)<br>
> +++ cfe/trunk/test/SemaCXX/member-expr.cpp Wed Jul 31 12:38:24 2013<br>
> @@ -79,16 +79,13 @@ namespace test5 {<br>
> };<br>
><br>
> void test0(int x) {<br>
> - x.A::foo<int>(); // expected-error {{'int' is not a structure or union}}<br>
<br>
</div></div>If you're removing these lines, you can probably remove these testN<br>
functions entirely (& perhaps rename test2 to test0 or test1).<br></blockquote><div> </div><div>Not sure why these two lines were deleted (or why I didn't notice them in the diff before committing the change) as they were unaffected by my code changes. Restored as part of r187521.</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5"><br>
> }<br>
><br>
> void test1(A *x) {<br>
> - x.A::foo<int>(); // expected-error {{'test5::A *' is a pointer}}<br>
> }<br>
><br>
> void test2(A &x) {<br>
> - x->A::foo<int>(); // expected-error {{'test5::A' is not a pointer}} \<br>
> - // expected-note {{did you mean to use '.' instead?}}<br>
> + x->A::foo<int>(); // expected-error {{'test5::A' is not a pointer; maybe you meant to use '.'?}}<br>
> }<br>
> }<br>
><br>
> @@ -182,7 +179,36 @@ namespace PR15045 {<br>
><br>
> int f() {<br>
> Cl0 c;<br>
> - return c->a; // expected-error {{member reference type 'PR15045::Cl0' is not a pointer}} \<br>
> - // expected-note {{did you mean to use '.' instead?}}<br>
> + return c->a; // expected-error {{member reference type 'PR15045::Cl0' is not a pointer; maybe you meant to use '.'?}}<br>
> + }<br>
> +<br>
> + struct bar {<br>
> + void func(); // expected-note {{'func' declared here}}<br>
> + };<br>
> +<br>
> + struct foo {<br>
> + bar operator->(); // expected-note 2 {{'->' applied to return value of the operator->() declared here}}<br>
> + };<br>
> +<br>
> + template <class T> void call_func(T t) {<br>
> + t->func(); // expected-error-re 2 {{member reference type 'PR15045::bar' is not a pointer$}} \<br>
> + // expected-note {{did you mean to use '.' instead?}}<br>
> + }<br>
> +<br>
> + void test_arrow_on_non_pointer_records() {<br>
> + bar e;<br>
> + foo f;<br>
> +<br>
> + // Show that recovery has happened by also triggering typo correction<br>
<br>
</div></div>Recovery has happened - but has the fixit actually been issued? We<br>
have tests for fixit hints that either test the machine readable form<br>
and/or rerun the compiler on the fixit'd code (I assume the machine<br>
readable tests are better, but not sure what the general<br>
consensus/plan is). See the tests under test/FixIt.<br></blockquote><div><br></div><div>The comment was based on the behavior prior to this commit where a fixit was being provided but recovery wasn't being attempted, so even though fixit mode with the right flags would fix the code, clang would stop analyzing the statement after diagnosing the "->" and things like typo correction on the member name would not occur (until the "->" had been changed to "." and clang was rerun on the code). Also added a small FixIt test to ensure that part works too. ;)</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5"><br>
> + e->Func(); // expected-error {{member reference type 'PR15045::bar' is not a pointer; maybe you meant to use '.'?}} \<br>
> + // expected-error {{no member named 'Func' in 'PR15045::bar'; did you mean 'func'?}}<br>
> +<br>
> + // Make sure a fixit isn't given in the case that the '->' isn't actually<br>
> + // the problem (the problem is with the return value of an operator->).<br>
> + f->func(); // expected-error-re {{member reference type 'PR15045::bar' is not a pointer$}}<br>
> +<br>
> + call_func(e); // expected-note {{in instantiation of function template specialization 'PR15045::call_func<PR15045::bar>' requested here}}<br>
> +<br>
> + call_func(f); // expected-note {{in instantiation of function template specialization 'PR15045::call_func<PR15045::foo>' requested here}}<br>
> }<br>
> }<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=cfe-commits@cs.uiuc.edu&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>