r225389 - Handle OpaqueValueExprs more intelligently in the TransformTypos tree
Richard Smith
richard at metafoo.co.uk
Tue Jan 20 17:05:43 PST 2015
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.
>> >>> +
>> >>> 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
>> >>
>> >>
>> >
>
>
More information about the cfe-commits
mailing list