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

[Kaelyn Takata]

On Fri, Jun 27, 2014 at 10:42 AM, Kaelyn Takata <rikka at google.com> wrote:

> 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?
>
ping ^^^^


>
>>
>> +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/20140707/23e3dbce/attachment.html>