<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Nov 5, 2013 at 12:14 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="h5">On Tue, Nov 5, 2013 at 11:54 AM, Kaelyn Uhrain <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div>On Tue, Nov 5, 2013 at 11:37 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>On Tue, Nov 5, 2013 at 10:40 AM, Kaelyn Uhrain <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>On Tue, Nov 5, 2013 at 10:21 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>On Tue, Nov 5, 2013 at 9:56 AM, Kaelyn Uhrain <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div>On Mon, Nov 4, 2013 at 6:37 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<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><p dir="ltr"><br>
On 4 Nov 2013 11:05, "Kaelyn Uhrain" <<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>> wrote:<br>
><br>
> Author: rikka<br>
> Date: Mon Nov 4 12:59:34 2013<br>
> New Revision: 194002<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=194002&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=194002&view=rev</a><br>
> Log:<br>
> Try to correct a mistyped "-" or ">" to "->" for some C++ cases.<br>
><br>
> Similar C code isn't caught as it seems to hit a different code path.<br>
> Also, as the check is only done for record pointers, cases involving<br>
> an overloaded operator-> are not handled either. Note that the reason<br>
> this check is done in the parser instead of Sema is not related to<br>
> having enough knowledge about the current state as it is about being<br>
> able to fix up the parser's state to be able to recover and traverse the<br>
> correct code paths.<br>
><br>
> Modified:<br>
> cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td<br>
> cfe/trunk/lib/Parse/ParseExpr.cpp<br>
> cfe/trunk/test/SemaCXX/member-expr.cpp<br>
><br>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=194002&r1=194001&r2=194002&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=194002&r1=194001&r2=194002&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)<br>
> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Mon Nov 4 12:59:34 2013<br>
> @@ -499,6 +499,9 @@ def ext_abstract_pack_declarator_parens<br>
> def err_function_is_not_record : Error<<br>
> "unexpected '%select{.|->}0' in function call; perhaps remove the "<br>
> "'%select{.|->}0'?">;<br>
> +def err_mistyped_arrow_in_member_access : Error<<br>
> + "use of undeclared identifier %0; did you mean '->' instead of "<br>
> + "'%select{-|>}1'?">;<br>
><br>
> // C++ derived classes<br>
> def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">;<br>
><br>
> Modified: cfe/trunk/lib/Parse/ParseExpr.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseExpr.cpp?rev=194002&r1=194001&r2=194002&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseExpr.cpp?rev=194002&r1=194001&r2=194002&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Parse/ParseExpr.cpp (original)<br>
> +++ cfe/trunk/lib/Parse/ParseExpr.cpp Mon Nov 4 12:59:34 2013<br>
> @@ -166,6 +166,46 @@ ExprResult Parser::ParseAssignmentExpres<br>
> ExprResult LHS = ParseCastExpression(/*isUnaryExpression=*/false,<br>
> /*isAddressOfOperand=*/false,<br>
> isTypeCast);<br>
> +<br>
> + // Check for a possible typo of "-" or ">" instead of "->" after a<br>
> + // pointer to a struct or class, while recovery is still possible.<br>
> + if (LHS.isUsable() && (Tok.is(tok::minus) || Tok.is(tok::greater))) {<br>
> + QualType LHSType = LHS.get()->getType();<br>
> + const RecordType *Pointee =<br>
> + LHSType->isPointerType()<br>
> + ? LHSType->getPointeeType()->getAsStructureType()<br>
> + : 0;<br>
> + const RecordDecl *RD = Pointee ? Pointee->getDecl() : 0;<br>
> + const Token &NextTok = NextToken();<br>
> + if (RD && NextTok.is(tok::identifier)) {<br>
> + UnqualifiedId Name;<br>
> + CXXScopeSpec ScopeSpec;<br>
> + SourceLocation TemplateKWLoc;<br>
> + NoTypoCorrectionCCC NoTCValidator;<br>
> + Name.setIdentifier(NextTok.getIdentifierInfo(), NextTok.getLocation());<br>
> + Sema::SFINAETrap Trap(Actions);<br>
> + ExprResult Res =<br>
> + Actions.ActOnIdExpression(getCurScope(), ScopeSpec, TemplateKWLoc,<br>
> + Name, false, false, &NoTCValidator);<br>
> + if (Res.isInvalid()) {</p>
</div></div><p dir="ltr">What happens if the next token is ( or :: or similar, and this identifier is not an id-expression by itself? For instance, p-f(x) might find f by adl.</p></blockquote></div></div><div>Assuming (1) "f" is both the name of a member of "p" and something not found through a simple lookup but through adl or similar, and (2) the pointer arithmetic/comparison on an object pointer is intended, then yes this will probably do the wrong thing. And while it's not great, working around that should be pretty trivial (e.g. adding parentheses around the rhs of the "-" or ">").</div>
</div></div></div></blockquote><div><br></div></div></div><div>OK, so this means we reject valid code; that's not OK -- please fix or revert. Here's one example of valid code this patch rejects:</div><div><br></div>
<div>struct S { int f(...); };</div>
<div>namespace X { struct T {}; int f(T); }</div><div>S *g(S *p) { return p - f(X::T()); }<br></div><div><br></div><div>This also produces a diagnostic with garbage in it:</div><div><br></div><div> error: use of undeclared identifier '������'; did you mean '->' instead of '-'?<br>
</div></div></div></div></blockquote><div><br></div></div></div><div>Reverted the commit until a way to handle cases like these can be figured out. You wouldn't happen to have any suggestions for how to detect cases like the one you gave, do you? The help would be much appreciated.</div>
</div></div></div></blockquote><div><br></div></div></div><div>The easy thing to do would be to form a blacklist of problematic next tokens. This would need to include at least '(' and '::' -- though blacklisting '(' seems to remove a lot of the value from this diagnostic, sadly.</div>
<div><br></div><div>I wonder if you can move this into ParseRHSOfBinaryExpression and wrap some kind of diagnostic trap/buffer around the ParseAssignmentExpression call there. Then, if that fails, try to interpret the - or > as a -> instead. You'd need to make sure that the diagnostic trap is cheap enough to not cause problems in such a hot code path, though.</div>
<div><br></div><div>Maybe you could introduce a mechanism to register a callback to be invoked if an error is produced; from that mechanism, you could check whether interpreting the expression as a member expression would work. If it would, produce your new diagnostic from there and suppress all further diagnostics until you get back to ParseRHSOfBinaryExpr.</div>
</div></div></div></blockquote><div><br></div></div></div><div>Right now I'm looking into having a way to (optionally) capture multiple diagnostics for later emission, with the intent of calling ParseRHSOfBinaryExpr as if the "-" or ">" is intended, and if diagnostics are captured then retry with an arrow instead similar to what my original patch does (including the SFINAETrap). Then if that fails, emit the captured diagnostics and pretend that the attempted arrow substitution never happened. Plus I remember encountering other situations where I wished I could capture multiple diagnostics for possible later emission if an alternate recovery attempt failed, so I think that part may prove quite useful outside of this particular situation. If I understand your suggestion correctly, I think my approach is quite similar but without having to ensure the callback is invoked prior to emitting a diagnostic wherever that diagnostic might occur and without having the callback be able to temporarily reset the state of the parser and without the callback needing a way to revert/abort part of the current call chain.</div>
</div></div></div></blockquote><div><br></div></div></div><div>My idea was not to revert / abort the current call chain, but simply to suppress all the diagnostics from it. That way we can avoid needing to buffer up diagnostics in the more common case that the '-' or '>' was not a typo. And my intent was that the callback would be invoked by Sema's diagnostic machinery, so it shouldn't require any changes outside that -- and you're going to need changes there either way, if you want to implement a buffering diagnostics mechanism. I don't think we would need to touch the parser state, but we might possibly need to restore Sema's context, or at least check that building a member expression doesn't look at the context.</div>
<div><br></div><div>If you think that implementing a diagnostic buffering system will work out better, I'm OK with that, but I'm more concerned about the possible performance cost with that approach.</div></div></div>
</div></blockquote><div><br></div><div>I'd only do the buffering in the case where the pointer is for a record and the next token is "-" or ">" and the buffering would just be to capturing the diagnostic messages instead of letting them be printed (and hence most of the overhead is only in the error case). Also, I'm not sure how the callback approach would handle backing up the parser to traverse a different code path to effectively call ParsePostfixExpressionSuffix then make the call to ParseRHSOfBinaryExpr on a different set of tokens, in order to both determine whether the substitution of an arrow will work and to recover if it does</div>
<div>. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><div><br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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 dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><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">
<p dir="ltr">Also, in the case where this succeeds, we should annotate the produced expression onto the token stream to avoid reparsing it later.</p></blockquote></div><div>I'm not sure what you're saying here... care to explain?</div>
</div></div></div></blockquote><div><br></div></div><div>If you're going to the effort to produce an Expr from the identifier, please replace the tokens you used with a tok::annot_primary_expr token so that we won't need to redo the lookup and recreate the Expr when we actually parse the RHS.</div>
</div></div></div></blockquote><div><br></div></div><div> From my understanding, when the correction is performed, the RHS parsed by ParseRHSOfBinaryExpression is for what comes after the member expr (so after "p->f" which "p-f" was corrected to)--and the parsing has no more overlap than if the arrow hadn't been mistyped. Or are you referring to the RHS of the arrow and the call to ParsePostfixExpressionSuffix, in which case the lookup of the member expr has to be done anyway since the identifier lookup had failed?</div>
</div></div></div></blockquote><div><br></div></div><div>The case I'm referring to is when the correction is *not* performed, and we've already correctly parsed the id-expression that follows. In that case, the current approach would throw away its Expr*, and then immediately re-parse the exact same subexpression. But if the approach I suggested above works, this is moot because there's no reparsing involved.</div>
</div></div></div></blockquote><div><br></div></div><div>Ah, I see what you're saying now. I misunderstood which part you were talking about "succeeding" (thanks a lot gmail...).</div><div><div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>
<div> </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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><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>
<p dir="ltr">> + Token OpTok = Tok;<br>
> + Tok.setKind(tok::arrow);<br>
> + PP.EnableBacktrackAtThisPos();<br>
> + Res = ParsePostfixExpressionSuffix(LHS);<br>
> + if (Res.isUsable()) {<br>
> + LHS = Res;<br>
> + PP.CommitBacktrackedTokens();<br>
> + Diag(OpTok, diag::err_mistyped_arrow_in_member_access)<br>
> + << NextTok.getIdentifierInfo() << OpTok.is(tok::greater)<br>
> + << FixItHint::CreateReplacement(OpTok.getLocation(), "->");<br>
> + } else {<br>
> + Tok = OpTok;<br>
> + PP.Backtrack();<br>
> + }<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> return ParseRHSOfBinaryExpression(LHS, prec::Assignment);<br>
> }<br>
><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=194002&r1=194001&r2=194002&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/member-expr.cpp?rev=194002&r1=194001&r2=194002&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/SemaCXX/member-expr.cpp (original)<br>
> +++ cfe/trunk/test/SemaCXX/member-expr.cpp Mon Nov 4 12:59:34 2013<br>
> @@ -224,3 +224,16 @@ namespace pr16676 {<br>
> .i; // expected-error {{member reference type 'pr16676::S *' is a pointer; maybe you meant to use '->'}}<br>
> }<br>
> }<br>
> +<br>
> +namespace PR9054 {<br>
> +struct Foo {<br>
> + void bar(int);<br>
> + int fiz;<br>
> +};<br>
> +<br>
> +int test(struct Foo *foo) {<br>
> + foo-bar(5); // expected-error {{use of undeclared identifier 'bar'; did you mean '->' instead of '-'?}}<br>
> + foo>baz(4); // expected-error-re {{use of undeclared identifier 'baz'$}}<br>
> + return foo>fiz; // expected-error {{use of undeclared identifier 'fiz'; did you mean '->' instead of '>'?}}<br>
> +}<br>
> +}<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</p>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>