r225389 - Handle OpaqueValueExprs more intelligently in the TransformTypos tree
Kaelyn Takata
rikka at google.com
Tue Jan 20 16:33:50 PST 2015
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.
> >>> +
> >>> 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/20150120/d6ac0367/attachment.html>
More information about the cfe-commits
mailing list