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
