<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 20, 2015 at 5:05 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">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 class="HOEnZb"><div class="h5">On Tue, Jan 20, 2015 at 4:33 PM, Kaelyn Takata <<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>
><br>
><br>
> On Tue, Jan 20, 2015 at 3:26 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>><br>
> wrote:<br>
>><br>
>> On Tue, Jan 20, 2015 at 1:28 PM, Kaelyn Takata <<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>
>> ><br>
>> ><br>
>> > On Wed, Jan 14, 2015 at 7:36 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Wed, Jan 7, 2015 at 1:16 PM, Kaelyn Takata <<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>
>> >>><br>
>> >>> Author: rikka<br>
>> >>> Date: Wed Jan 7 15:16:39 2015<br>
>> >>> New Revision: 225389<br>
>> >>><br>
>> >>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=225389&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=225389&view=rev</a><br>
>> >>> Log:<br>
>> >>> Handle OpaqueValueExprs more intelligently in the TransformTypos tree<br>
>> >>> transform.<br>
>> >>><br>
>> >>> Also diagnose typos in the initializer of an invalid C++ declaration.<br>
>> >>> Both issues were hit using the same line of test code, depending on<br>
>> >>> whether the code was treated as C or C++.<br>
>> >>><br>
>> >>> Fixes PR22092.<br>
>> >>><br>
>> >>> Modified:<br>
>> >>> cfe/trunk/lib/Sema/SemaDecl.cpp<br>
>> >>> cfe/trunk/lib/Sema/SemaExprCXX.cpp<br>
>> >>> cfe/trunk/test/Sema/typo-correction.c<br>
>> >>> cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp<br>
>> >>><br>
>> >>> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp<br>
>> >>> URL:<br>
>> >>><br>
>> >>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=225389&r1=225388&r2=225389&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=225389&r1=225388&r2=225389&view=diff</a><br>
>> >>><br>
>> >>><br>
>> >>> ==============================================================================<br>
>> >>> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)<br>
>> >>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Wed Jan 7 15:16:39 2015<br>
>> >>> @@ -8574,8 +8574,10 @@ void Sema::AddInitializerToDecl(Decl *Re<br>
>> >>> bool DirectInit, bool<br>
>> >>> TypeMayContainAuto) {<br>
>> >>> // If there is no declaration, there was an error parsing it. Just<br>
>> >>> ignore<br>
>> >>> // the initializer.<br>
>> >>> - if (!RealDecl || RealDecl->isInvalidDecl())<br>
>> >>> + if (!RealDecl || RealDecl->isInvalidDecl()) {<br>
>> >>> + CorrectDelayedTyposInExpr(Init);<br>
>> >>> return;<br>
>> >>> + }<br>
>> >>>:61<br>
>> >>> if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(RealDecl)) {<br>
>> >>> // With declarators parsed the way they are, the parser cannot<br>
>> >>><br>
>> >>> Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp<br>
>> >>> URL:<br>
>> >>><br>
>> >>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=225389&r1=225388&r2=225389&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=225389&r1=225388&r2=225389&view=diff</a><br>
>> >>><br>
>> >>><br>
>> >>> ==============================================================================<br>
>> >>> --- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)<br>
>> >>> +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Wed Jan 7 15:16:39 2015<br>
>> >>> @@ -6141,6 +6141,12 @@ public:<br>
>> >>><br>
>> >>> ExprResult TransformLambdaExpr(LambdaExpr *E) { return Owned(E); }<br>
>> >>><br>
>> >>> + ExprResult TransformOpaqueValueExpr(OpaqueValueExpr *E) {<br>
>> >>> + if (Expr *SE = E->getSourceExpr())<br>
>> >>> + return TransformExpr(SE);<br>
>> >>> + return BaseTransform::TransformOpaqueValueExpr(E);<br>
>> >>> + }<br>
>> >><br>
>> >><br>
>> >> Hmm, can you instead handle this by overriding TreeTransform's<br>
>> >> AlreadyTransformed:<br>
>> >><br>
>> >> bool AlreadyTransformed(QualType T) {<br>
>> >> return T.isNull() || T->isInstantiationDependentType();<br>
>> >> }<br>
>> >><br>
>> >> (Since we treat a TypoExpr as being dependent, it must lead to an<br>
>> >> instantiation-dependent type.)<br>
>> ><br>
>> ><br>
>> > That fixes the TreeTransform assertion reported in PR22092, but it isn't<br>
>> > sufficient for the TypoExpr nested inside of the OpaqueValueExpr to be<br>
>> > seen<br>
>> > as the default TransformOpaqueValueExpr just returns the original<br>
>> > OpaqueValueExpr if the assertion doesn't fail. So instead the test case<br>
>> > added to typo-correction.c fails in a different way:<br>
>> ><br>
>> > error: 'error' diagnostics expected but not seen:<br>
>> > File ~/llvm/tools/clang/test/Sema/typo-correction.c Line 13: use of<br>
>> > undeclared identifier 'b'<br>
>> > error: 'error' diagnostics seen but not expected:<br>
>> > (frontend): used type '<dependent type>' where arithmetic, pointer, or<br>
>> > vector type is required<br>
>> > clang: ../tools/clang/lib/Sema/Sema.cpp:249: clang::Sema::~Sema():<br>
>> > Assertion<br>
>> > `DelayedTypos.empty() && "Uncorrected typos!"' failed.<br>
>> > [followed by the stack trace for the failed assertion]<br>
>> ><br>
>> > Overriding the definition of AlreadyTransformed just for to get an<br>
>> > assertion<br>
>> > in one Tranform*Expr method to pass also feels a bit... heavy-handed to<br>
>> > me.<br>
>><br>
>> The idea is that the OpaqueValueExpr is "owned" by some other AST<br>
>> node, and transforming that other AST node should cause the source<br>
>> expr of the OVE to be transformed and should recreate the OVE itself.<br>
>> So the TreeTransform should *never* see an OVE that needs<br>
>> transformation (actually, it should probably never see one at all...).<br>
>> In any case, it can't know how to transform one, because that's a<br>
>> non-local property and depends on the owner of the OVE.<br>
>><br>
>> For a BinaryConditionalOperator, we transform the "common" expr (not<br>
>> an OVE) and the RHS expr (also not an OVE). I ran your patch against<br>
>> this testcase:<br>
>><br>
>> int foobar; int f() { return goobar ?: 1; }<br>
>><br>
>> ... and your TransformOpaqueValueExpr function was not called. So I<br>
>> think that part is entirely a red herring.<br>
>><br>
>> In your testing, did you revert just the TransformOpaqueValueExpr part<br>
>> of this patch or also the change to SemaDecl (which I think was the<br>
>> real fix here)?<br>
><br>
><br>
> In my testing I only replaced the TransformOpaqueValueExpr with your<br>
> suggested AlreadyTransformed. Try testing the example added to<br>
> test/Sema/typo-correction.c, as this problem seems to be specific to the C<br>
> code path (the change to SemaDecl fixes the same example [which is pulled<br>
> from the PR] when it is treated as C++ code). Having "a = b ? : 0" at the<br>
> top level of a file causes the TreeTransform to see an OpaqueValueExpr when<br>
> the file is parsed as C code but not when it is parsed as C++ code.<br>
<br>
</div></div>For<br>
<br>
int foobar; a = goobar ?: 4;<br>
<br>
I get<br>
<br>
<stdin>:1:13: warning: type specifier missing, defaults to 'int'<br>
[-Wimplicit-int]<br>
int foobar; a = goobar ?: 4;<br>
^<br>
<stdin>:1:17: error: use of undeclared identifier 'goobar'; did you<br>
mean 'foobar'?<br>
int foobar; a = goobar ?: 4;<br>
^~~~~~<br>
foobar<br>
<stdin>:1:5: note: 'foobar' declared here<br>
int foobar; a = goobar ?: 4;<br>
^<br>
<stdin>:1:24: error: incompatible operand types ('<dependent type>' and 'int')<br>
int foobar; a = goobar ?: 4;<br>
^ ~<br>
<br>
... which suggests our error recovery isn't working properly here.<br>
<br>
Digging into this further, the issue is that the wrong thing is<br>
happening at the start of CheckConditionalOperands:<br>
<br>
if (!getLangOpts().CPlusPlus) {<br>
// C cannot handle TypoExpr nodes on either side of a binop because it<br>
// doesn't handle dependent types properly, so make sure any TypoExprs have<br>
// been dealt with before checking the operands.<br>
ExprResult CondResult = CorrectDelayedTyposInExpr(Cond);<br>
if (!CondResult.isUsable()) return QualType();<br>
Cond = CondResult;<br>
}<br>
<br>
This is not correct for a BinaryConditionalOperator; typo-correction<br>
should be applied to the common expression, not to the condition. The<br>
right thing to do would be to move this code up into<br>
ActOnConditionalOp (which should be the only caller of<br>
CheckConditionalOperands in C mode), to before we create the<br>
OpaqueValueExprs.<br></blockquote><div><br></div><div>Ah, I see! Thanks for pointing me back in the right direction. Attached is a patch that drops the custom TransformOpaqueValueExpr and hoists the typo correction of the conditional to ActOnConditionalOp before the check for a binary conditional operator. If it looks good to you, I'll submit it and propose the commit for the 3.6 branch.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><span style="color:black"> </span><br></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
>> >>> +<br>
>> >>> ExprResult Transform(Expr *E) {<br>
>> >>> ExprResult Res;<br>
>> >>> while (true) {<br>
>> >>><br>
>> >>> Modified: cfe/trunk/test/Sema/typo-correction.c<br>
>> >>> URL:<br>
>> >>><br>
>> >>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/typo-correction.c?rev=225389&r1=225388&r2=225389&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/typo-correction.c?rev=225389&r1=225388&r2=225389&view=diff</a><br>
>> >>><br>
>> >>><br>
>> >>> ==============================================================================<br>
>> >>> --- cfe/trunk/test/Sema/typo-correction.c (original)<br>
>> >>> +++ cfe/trunk/test/Sema/typo-correction.c Wed Jan 7 15:16:39 2015<br>
>> >>> @@ -9,3 +9,6 @@ void PR21656() {<br>
>> >>> float x;<br>
>> >>> x = (float)arst; // expected-error-re {{use of undeclared<br>
>> >>> identifier<br>
>> >>> 'arst'{{$}}}}<br>
>> >>> }<br>
>> >>> +<br>
>> >>> +a = b ? : 0; // expected-warning {{type specifier missing, defaults<br>
>> >>> to<br>
>> >>> 'int'}} \<br>
>> >>> + // expected-error {{use of undeclared identifier 'b'}}<br>
>> >>><br>
>> >>> Modified: cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp<br>
>> >>> URL:<br>
>> >>><br>
>> >>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp?rev=225389&r1=225388&r2=225389&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp?rev=225389&r1=225388&r2=225389&view=diff</a><br>
>> >>><br>
>> >>><br>
>> >>> ==============================================================================<br>
>> >>> --- cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp (original)<br>
>> >>> +++ cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp Wed Jan 7<br>
>> >>> 15:16:39 2015<br>
>> >>> @@ -152,3 +152,8 @@ namespace PR21947 {<br>
>> >>> int blue; // expected-note {{'blue' declared here}}<br>
>> >>> __typeof blur y; // expected-error {{use of undeclared identifier<br>
>> >>> 'blur'; did you mean 'blue'?}}<br>
>> >>> }<br>
>> >>> +<br>
>> >>> +namespace PR22092 {<br>
>> >>> +a = b ? : 0; // expected-error {{C++ requires a type specifier for<br>
>> >>> all<br>
>> >>> declarations}} \<br>
>> >>> + // expected-error-re {{use of undeclared identifier<br>
>> >>> 'b'{{$}}}}<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>
>> >><br>
>> >><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>