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

Kaelyn Takata rikka at google.com
Tue Sep 16 15:01:38 PDT 2014


Thanks for reviewing this, David!

On Fri, Sep 5, 2014 at 2:22 PM, David Blaikie <dblaikie at gmail.com> wrote:

> Looks mostly reasonable.
>
> The changes to include/clang/Sema/SemaInternal.h seem unrelated to this
> patch, though - perhaps they're meant for some other patch? The
> TypoDiagnosticGenerator too, is an isolated/unrelated change.
>

Yup, those look like a few pieces that got added to the wrong patch when I
was cleaning up and shuffling around and squashing commits into more
reasonable patches. They should be in patch #5 that starts adding the
infrastructure for handling a TypoExpr (of which TypoDiagnosticGenerator is
a part).

>
> It might be worth adding a test case for statement printing
> (StmtPrinter.cpp) with typos - if you can cause the TypoExpr to still be
> there when you do an AST Dump? (perhaps on invalid code? Or do you always
> transform the TypoExpr into /something/ else, even if you can't suggest a
> specific function to call?)
>

I'm not sure how to test that, even ignoring the fact that nothing in this
patch generates a TypoExpr. Also, the TypoExpr is a placeholder that should
either be replaced with a correction or an ExprError (with appropriate
diagnostic) before anything is truly done with the full Expr, so I don't
think it is possible to dump the AST--even with invalid input--before all
of the TypoExprs have been replaced (or at least without it being a bug in
the code where a path was hit that failed to handle TypoExprs). While it
may change in the future for use with something like code completion, as of
now TypoExprs shouldn't ever be exposed and could not meaningfully be
dumped and re-read as most of their state is saved in the specific Sema
instance (e.g. all of the namespaces currently visible to the translation
units, and all of the possible corrections that couldn't be immediately
discarded e.g. by virtue of the identifier being way too different).

>
> I'm assuming all the other changes are just necessary bits & Pieces to
> adding a new expr type? (eg: if I removed some of them, things would
> break/fail to compile/etc)
>

Yeah, much of this patch is just the boilerplate associated with keeping
compilers happy when adding a new Expr type.


>
>
> On Tue, Aug 26, 2014 at 11:04 AM, Kaelyn Takata <rikka at google.com> wrote:
>
>> +dblaikie
>>
>>
>> On Thu, Jul 31, 2014 at 1:20 PM, Kaelyn Takata <rikka at google.com> wrote:
>>
>>> ---
>>>  include/clang/AST/DataRecursiveASTVisitor.h |  1 +
>>>  include/clang/AST/Expr.h                    | 18 ++++++++++++++++++
>>>  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/TreeTransform.h                    |  6 ++++++
>>>  lib/Serialization/ASTReaderStmt.cpp         |  4 ++++
>>>  lib/Serialization/ASTWriterStmt.cpp         |  6 ++++++
>>>  lib/StaticAnalyzer/Core/ExprEngine.cpp      |  1 +
>>>  tools/libclang/CXCursor.cpp                 |  1 +
>>>  18 files changed, 62 insertions(+), 1 deletion(-)
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140916/19aee82d/attachment.html>


More information about the cfe-commits mailing list