r225389 - Handle OpaqueValueExprs more intelligently in the TransformTypos tree
Kaelyn Takata
rikka at google.com
Tue Jan 27 10:42:57 PST 2015
Submitted as r227220. Thanks again for your help, Richard!
On Mon, Jan 26, 2015 at 5:19 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
> LGTM for trunk and branch.
>
> On Mon, Jan 26, 2015 at 3:46 PM, Kaelyn Takata <rikka at google.com> wrote:
> >
> >
> > On Tue, Jan 20, 2015 at 5:05 PM, Richard Smith <richard at metafoo.co.uk>
> > wrote:
> >>
> >> On Tue, Jan 20, 2015 at 4:33 PM, Kaelyn Takata <rikka at google.com>
> wrote:
> >> >
> >> >
> >> > On Tue, Jan 20, 2015 at 3:26 PM, Richard Smith <richard at metafoo.co.uk
> >
> >> > wrote:
> >> >>
> >> >> On Tue, Jan 20, 2015 at 1:28 PM, Kaelyn Takata <rikka at google.com>
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Wed, Jan 14, 2015 at 7:36 PM, Richard Smith
> >> >> > <richard at metafoo.co.uk>
> >> >> > wrote:
> >> >> >>
> >> >> >> On Wed, Jan 7, 2015 at 1:16 PM, Kaelyn Takata <rikka at google.com>
> >> >> >> 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
> >> >> >>> cfe-commits at cs.uiuc.edu
> >> >> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >> >> >>
> >> >> >>
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150127/4c96721b/attachment.html>
More information about the cfe-commits
mailing list