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