[PATCH v2 2/9] Add the initial TypoExpr AST node for delayed typo correction.

[Kaelyn Takata]

On Fri, Jun 27, 2014 at 7:31 AM, Richard Smith <richard at metafoo.co.uk>
wrote:

> +  TypoExpr(std::unique_ptr<TypoCorrectionConsumer> TCC,
> +           std::unique_ptr<TypoDiagnosticGenerator> TDG);
> +  child_range children() { return child_range(); }
> +  SourceLocation getLocStart() const LLVM_READONLY { return
> SourceLocation(); }
> +  SourceLocation getLocEnd() const LLVM_READONLY { return
> SourceLocation(); }
> +
> +  std::unique_ptr<TypoCorrectionConsumer> Consumer;
> +  std::unique_ptr<TypoDiagnosticGenerator> DiagHandler;
>
> These unique_ptrs will be leaked; AST nodes don't get destroyed. Please
> allocate your handlers on the ASTContext instead.
>

Well @#$%. I'm assuming that allocating these on the ASTContext would mean
calling e.g. "new (SemaRef.Context) TypoCorrectionConsumer(...)" instead of
"llvm::make_unique<TypoCorrectionConsumer>(...)" and passing around raw
pointers instead of unique_ptrs?


>
> +TypoExpr::TypoExpr(std::unique_ptr<TypoCorrectionConsumer> TCC,
> +                   std::unique_ptr<TypoDiagnosticGenerator> TDG)
> +    : Expr(TypoExprClass, TCC->getContext().DependentTy, VK_LValue,
> OK_Ordinary,
> +           /*isTypeDependent*/ true,
> +           /*isValueDependent*/ true,
> +           /*isInstantiationDependent*/ true,
> +           /*containsUnexpandedParameterPack*/ false),
>
> Hmm, parameter packs are an interesting twist on this. Suppose we have:
>
>   template<typename...Thing> void f(Thing ...thing) { g(rhing...); }
>
> if we set containsUnexpandedParameterPack to false, we'll reject this in
> the initial parse. If we set it to true, we'll reject other stuff instead.
> =( Maybe we should suppress diagnosing an unexpanded pack if we've got an
> active typo correction?
>

Umm... yeah... having not really worked with parameter packs or looked at
any (failing) typo-correction unit tests involving parameter packs yet, I
don't know what the right answer is. Nor do I quite know where the boundary
of containsUnexpandedParameterPack being true would fall relative to an
expression placeholder that represents a possibly-qualified identifier and
the hooks needed to expand the identifier into an expression that fits in
where the identifier was seen.


>
>  template<typename Derived>
>  ExprResult
> +TreeTransform<Derived>::TransformTypoExpr(TypoExpr *E) {
> +  assert(getDerived().AlreadyTransformed(E->getType()) &&
> +         "typo-correction placeholder expression requires
> transformation");
> +  return Owned(E);
> +}
>
> Can we really not get here with a TypoExpr still in the tree? We have
> enough TreeTransforms these days that I find this a bit surprising. Maybe
> TreeTransform should just preserve TypoExprs by default.
>

D'oh, you're right. When I added all of the (Transform|Visit)TypoExpr
methods to the various classes like this one and ASTStmtReader in order to
make the compiler happy, I applied pretty much the same code or mentality
to all of the methods: "a TypoExpr is internal to Sema so shouldn't ever
hit these paths since it should never still be in the AST tree when the
tree is serialized, deserialized, etc)". I've dropped the assert here (and
the now-defunct call to Owned).


>
> +void ASTStmtReader::VisitTypoExpr(TypoExpr *E) {
> +  VisitExpr(E);
> +  // TODO: Figure out sane reader behavior for a TypoExpr, if necessary.
> +  assert(false && "Cannot read TypoExpr nodes");
> +}
>
> I think this should be unreachable; if things have gone so badly wrong
> that we couldn't resolve a TypoExpr, we shouldn't serialize it either.
>

I've replaced the three lines in the body with a call to llvm_unreachable().


>
> On Tue, Jun 17, 2014 at 5:29 PM, Kaelyn Takata <rikka at google.com> wrote:
>
>> ---
>>  include/clang/AST/DataRecursiveASTVisitor.h |  1 +
>>  include/clang/AST/Expr.h                    | 17 +++++++++++++++++
>>  include/clang/AST/RecursiveASTVisitor.h     |  1 +
>>  include/clang/Basic/StmtNodes.td            |  1 +
>>  include/clang/Sema/SemaInternal.h           |  3 +++
>>  include/clang/Sema/TypoCorrection.h         |  5 +++++
>>  lib/AST/Expr.cpp                            |  1 +
>>  lib/AST/ExprClassification.cpp              |  3 ++-
>>  lib/AST/ExprConstant.cpp                    |  1 +
>>  lib/AST/ItaniumMangle.cpp                   |  1 +
>>  lib/AST/StmtPrinter.cpp                     |  5 +++++
>>  lib/AST/StmtProfile.cpp                     |  4 ++++
>>  lib/Sema/SemaExceptionSpec.cpp              |  1 +
>>  lib/Sema/SemaLookup.cpp                     | 11 +++++++++++
>>  lib/Sema/TreeTransform.h                    |  8 ++++++++
>>  lib/Serialization/ASTReaderStmt.cpp         |  6 ++++++
>>  lib/Serialization/ASTWriterStmt.cpp         |  6 ++++++
>>  lib/StaticAnalyzer/Core/ExprEngine.cpp      |  1 +
>>  tools/libclang/CXCursor.cpp                 |  1 +
>>  19 files changed, 76 insertions(+), 1 deletion(-)
>>
>>
>> _______________________________________________
>> 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/20140627/8c95fd8f/attachment.html>