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

[Richard Smith]

On Mon, Jul 7, 2014 at 3:38 PM, Kaelyn Takata <rikka at google.com> wrote:

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

Yes, that's exactly what I had in mind.


>>
>>>
>>> +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/20140708/8e72b0cf/attachment.html>