<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 20, 2015 at 3:26 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: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 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>
>>> <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>
>>> --- 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>
</div></div>>>>:61<br>
<div><div class="h5">>>> 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>
>>> <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>
>>> --- 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 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(): 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 assertion<br>
> in one Tranform*Expr method to pass also feels a bit... heavy-handed to me.<br>
<br>
</div></div>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></blockquote><div><br></div><div>In my testing I only replaced the TransformOpaqueValueExpr with your suggested AlreadyTransformed. Try testing the example added to <span style="font-size:13px;color:rgb(34,34,34)">test/Sema/typo-</span><span style="font-size:13px;color:rgb(34,34,34)">correction.c, as this problem seems to be specific to the C code path (the change to SemaDecl fixes the same example [which is pulled from the PR] when it is treated as C++ code). Having "a = b ? : 0" at the top level of a file causes the TreeTransform to see an OpaqueValueExpr when the file is parsed as C code but not when it is parsed as C++ code.</span></div><div><span style="font-size:13px;color:rgb(34,34,34)"><br></span></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>
>>> +<br>
>>> ExprResult Transform(Expr *E) {<br>
>>> ExprResult Res;<br>
>>> while (true) {<br>
>>><br>
>>> Modified: cfe/trunk/test/Sema/typo-correction.c<br>
>>> URL:<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>
>>> --- 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 identifier<br>
>>> 'arst'{{$}}}}<br>
>>> }<br>
>>> +<br>
>>> +a = b ? : 0; // expected-warning {{type specifier missing, defaults 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>
>>> <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>
>>> --- 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 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>
</div></div></blockquote></div><br></div></div>