<div dir="ltr">Submitted as r227220. Thanks again for your help, Richard!<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 26, 2015 at 5:19 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">LGTM for trunk and branch.<br>
<div class="HOEnZb"><div class="h5"><br>
On Mon, Jan 26, 2015 at 3:46 PM, Kaelyn Takata <<a href="mailto:rikka@google.com">rikka@google.com</a>> wrote:<br>
><br>
><br>
> On Tue, Jan 20, 2015 at 5:05 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
> wrote:<br>
>><br>
>> On Tue, Jan 20, 2015 at 4:33 PM, Kaelyn Takata <<a href="mailto:rikka@google.com">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">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">rikka@google.com</a>><br>
>> >> wrote:<br>
>> >> ><br>
>> >> ><br>
>> >> > On Wed, Jan 14, 2015 at 7:36 PM, Richard Smith<br>
>> >> > <<a href="mailto:richard@metafoo.co.uk">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">rikka@google.com</a>><br>
>> >> >> 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">http://llvm.org/viewvc/llvm-project?rev=225389&view=rev</a><br>
>> >> >>> Log:<br>
>> >> >>> Handle OpaqueValueExprs more intelligently in the TransformTypos<br>
>> >> >>> tree<br>
>> >> >>> transform.<br>
>> >> >>><br>
>> >> >>> Also diagnose typos in the initializer of an invalid C++<br>
>> >> >>> 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>
>> >> >>><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">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>
>> >> >>> ==============================================================================<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.<br>
>> >> >>> 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>
>> >> >>><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">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>
>> >> >>> ==============================================================================<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>
>> >> >>><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<br>
>> >> > isn't<br>
>> >> > sufficient for the TypoExpr nested inside of the OpaqueValueExpr to<br>
>> >> > be<br>
>> >> > seen<br>
>> >> > as the default TransformOpaqueValueExpr just returns the original<br>
>> >> > OpaqueValueExpr if the assertion doesn't fail. So instead the test<br>
>> >> > 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,<br>
>> >> > 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<br>
>> >> > 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<br>
>> > C<br>
>> > code path (the change to SemaDecl fixes the same example [which is<br>
>> > pulled<br>
>> > from the PR] when it is treated as C++ code). Having "a = b ? : 0" at<br>
>> > the<br>
>> > top level of a file causes the TreeTransform to see an OpaqueValueExpr<br>
>> > when<br>
>> > the file is parsed as C code but not when it is parsed as C++ code.<br>
>><br>
>> 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<br>
>> '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<br>
>> 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>
><br>
><br>
> Ah, I see! Thanks for pointing me back in the right direction. Attached is a<br>
> patch that drops the custom TransformOpaqueValueExpr and hoists the typo<br>
> correction of the conditional to ActOnConditionalOp before the check for a<br>
> binary conditional operator. If it looks good to you, I'll submit it and<br>
> propose the commit for the 3.6 branch.<br>
>><br>
>><br>
>><br>
>> >> >>> +<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>
>> >> >>><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">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>
>> >> >>> ==============================================================================<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,<br>
>> >> >>> defaults<br>
>> >> >>> to<br>
>> >> >>> 'int'}} \<br>
>> >> >>> + // expected-error {{use of undeclared identifier<br>
>> >> >>> 'b'}}<br>
>> >> >>><br>
>> >> >>> Modified: cfe/trunk/test/SemaCXX/typo-correction-delayed.cpp<br>
>> >> >>> URL:<br>
>> >> >>><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">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>
>> >> >>> ==============================================================================<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<br>
>> >> >>> 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">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>
>> >> >><br>
>> >> >><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>