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. > >>> + > >>> 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
