Discussion:
[PATCH v3 0/9] Initial implementation of delayed typo correction.
(too old to reply)
Kaelyn Takata
2014-07-14 23:55:32 UTC
Permalink
This patch set adds a new node to the AST called a TypoExpr. It is used as a
placeholder to delay typo correction until a full expression is being finalized
(via Sema::ActOnFinishFullExpr), at which point a TreeTransform is run to
replace each TypoExpr by performing typo correction. This allows, for example,
to delay typo correction on a call to a class method until after the arguments
have been parsed, instead of trying to do the typo correction while looking up
the member before the parser has even reached the opening parenthesis. The
patches have been broken up such that clang builds and the tests pass after
each patch in the sequence has been applied. The first patch is included in
here despite being a simple, mechanical moving around of code (instead of
committing it directly) because it is unnecessary/useless outside of the rest
of the patch set.

v2:
* Unmunge a couple of patches that accidentally got merged together
(primarily #5 had incorrectly gotten squashed into #4).
* Pass unique_ptrs by value instead of by reference.
v3:
* Have the TypoCorrectionConsumer remember all TypoCorrections it has
returned instead of just the last one, and add the ability to restart/replay
returning those TypoCorrections.
* Rework TransformTypos to try all combinations of typo corrections when
multiple TypoExpr nodes are present within the same Expr.
* Add a helper method to TypoExpr to free the unique_ptr members once the
TypoExpr has been processed e.g by TransformTypos.
* A few other small changes in response to review feedback of patches 2 and 5.

Note that dealing with TypoExprs that weren't handled by a call to
ActOnFinishFullExpr is still missing. It will be coming in a subsequent patch
since I don't have a good reproduction for having LookupMemberExpr create a
TypoExpr that isn't subsequently dealt with by ActOnFinishFullExpr, while such
cases are common when having DiagnoseEmptyLookup call CorrectTypoDelayed on
behalf of ActOnFinishFullExpr.

Kaelyn Takata (9):
Move TypoCorrectionConsumer into a header.
Add the initial TypoExpr AST node for delayed typo correction.
Pass around CorrectionCandidateCallbacks as unique_ptrs so
TypoCorrectionConsumer can keep the callback around as long as
needed.
Have TypoCorrectionConsumer remember the TypoCorrections it returned.
Start adding the infrastructure for handling TypoExprs.
Add simple way for a CorrectionCandidateCallback to reject exact
matches of the typo.
Add another keyword-selection flag to CorrectionCandidateCallback.
Add a callback for recovering using a typo correction.
Wire up LookupMemberExpr to use the new TypoExpr.

include/clang/AST/DataRecursiveASTVisitor.h | 1 +
include/clang/AST/Expr.h | 23 ++
include/clang/AST/RecursiveASTVisitor.h | 1 +
include/clang/Basic/StmtNodes.td | 1 +
include/clang/Parse/Parser.h | 5 +-
include/clang/Sema/Sema.h | 53 ++-
include/clang/Sema/SemaInternal.h | 181 ++++++++-
include/clang/Sema/TypoCorrection.h | 47 ++-
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/Parse/ParseExpr.cpp | 8 +-
lib/Parse/ParseStmt.cpp | 6 +-
lib/Parse/ParseTentative.cpp | 8 +-
lib/Parse/Parser.cpp | 8 +-
lib/Sema/SemaCXXScopeSpec.cpp | 9 +-
lib/Sema/SemaDecl.cpp | 57 ++-
lib/Sema/SemaDeclCXX.cpp | 27 +-
lib/Sema/SemaDeclObjC.cpp | 21 +-
lib/Sema/SemaExceptionSpec.cpp | 1 +
lib/Sema/SemaExpr.cpp | 44 ++-
lib/Sema/SemaExprCXX.cpp | 117 ++++++
lib/Sema/SemaExprMember.cpp | 164 +++++++-
lib/Sema/SemaExprObjC.cpp | 18 +-
lib/Sema/SemaInit.cpp | 4 +-
lib/Sema/SemaLambda.cpp | 4 +-
lib/Sema/SemaLookup.cpp | 594 ++++++++++++++--------------
lib/Sema/SemaOpenMP.cpp | 7 +-
lib/Sema/SemaOverload.cpp | 23 +-
lib/Sema/SemaTemplate.cpp | 17 +-
lib/Sema/SemaTemplateVariadic.cpp | 8 +-
lib/Sema/TreeTransform.h | 6 +
lib/Serialization/ASTReaderStmt.cpp | 4 +
lib/Serialization/ASTWriterStmt.cpp | 6 +
lib/StaticAnalyzer/Core/ExprEngine.cpp | 1 +
test/SemaCXX/arrow-operator.cpp | 5 +-
test/SemaCXX/typo-correction-delayed.cpp | 32 ++
test/SemaCXX/typo-correction-pt2.cpp | 2 +-
test/SemaCXX/typo-correction.cpp | 10 +-
tools/libclang/CXCursor.cpp | 1 +
43 files changed, 1054 insertions(+), 485 deletions(-)
create mode 100644 test/SemaCXX/typo-correction-delayed.cpp
--
2.0.0.526.g5318336
Kaelyn Takata
2014-07-14 23:55:33 UTC
Permalink
This makes it available outside of SemaLookup.cpp, as
needed for the forthcoming TypoExpr AST node which will
keep a TypoCorrectionConsumer that provides the possible
typo corrections for that TypoExpr.
---
include/clang/Sema/SemaInternal.h | 148 +++++++++++++++++++++++++++++++++++-
lib/Sema/SemaLookup.cpp | 153 +-------------------------------------
2 files changed, 149 insertions(+), 152 deletions(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0001-Move-TypoCorrectionConsumer-into-a-header.patch
Type: text/x-patch
Size: 13463 bytes
Desc: not available
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140714/0b71bca9/attachment.bin>
Richard Smith
2014-07-25 20:48:09 UTC
Permalink
LGTM
Post by Kaelyn Takata
This makes it available outside of SemaLookup.cpp, as
needed for the forthcoming TypoExpr AST node which will
keep a TypoCorrectionConsumer that provides the possible
typo corrections for that TypoExpr.
---
include/clang/Sema/SemaInternal.h | 148
+++++++++++++++++++++++++++++++++++-
lib/Sema/SemaLookup.cpp | 153
+-------------------------------------
2 files changed, 149 insertions(+), 152 deletions(-)
_______________________________________________
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.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/af64fb9f/attachment.html>
Kaelyn Takata
2014-07-14 23:55:34 UTC
Permalink
---
include/clang/AST/DataRecursiveASTVisitor.h | 1 +
include/clang/AST/Expr.h | 20 ++++++++++++++++++++
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 | 16 ++++++++++++++++
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 +
19 files changed, 80 insertions(+), 1 deletion(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0002-Add-the-initial-TypoExpr-AST-node-for-delayed-typ.patch
Type: text/x-patch
Size: 11423 bytes
Desc: not available
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140714/775a35fc/attachment-0001.bin>
Richard Smith
2014-07-25 20:57:46 UTC
Permalink
Post by Kaelyn Takata
---
include/clang/AST/DataRecursiveASTVisitor.h | 1 +
include/clang/AST/Expr.h | 20 ++++++++++++++++++++
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 | 16 ++++++++++++++++
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 +
19 files changed, 80 insertions(+), 1 deletion(-)
The AST library is supposed to be self-contained; it's a layering violation
to put TypoExpr member definitions into Sema. Conversely, it'd be a
layering violation to put them into AST, since they currently delete Sema
types. To avoid this, I suggest:

* The TypoExpr class should instead store a single opaque 'void*' pointer,
and make no attempt to deallocate or otherwise manage it.
* Sema owns the pointee and is responsible for deleting it (you can delete
it and zero out the pointer on the TypoExpr at the point when you handle
the TypoExpr)

Alternatively, since you need Sema to track the live TypoExprs anyway, you
could have Sema maintain a map from TypoExpr* to whatever data you want to
store for the TypoExprs, and have the AST node be essentially stateless.

Incidentally, I wonder whether it'd be useful for TypoExpr to store the
typoed identifier, maybe as an IdentifierInfo*? (This'd allow you to
produce something more useful from the statement printer and ast dumper,
and we really don't care much how big a TypoExpr is, since we'll --
presumably -- never have many of them.) This might also be useful for
clients that want to turn off typo correction but keep the TypoExprs in the
AST.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/4c5df1bd/attachment-0001.html>
Kaelyn Takata
2014-07-25 22:21:28 UTC
Permalink
On Fri, Jul 25, 2014 at 1:57 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
Post by Kaelyn Takata
---
include/clang/AST/DataRecursiveASTVisitor.h | 1 +
include/clang/AST/Expr.h | 20 ++++++++++++++++++++
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 | 16 ++++++++++++++++
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 +
19 files changed, 80 insertions(+), 1 deletion(-)
The AST library is supposed to be self-contained; it's a layering
violation to put TypoExpr member definitions into Sema. Conversely, it'd be
a layering violation to put them into AST, since they currently delete Sema
I'd have actually preferred to keep TypoExpr out of the AST library
altogether since it is a synthetic node generated by Sema for use by Sema
and really shouldn't ever be seen/touched outside of Sema, as it is simply
a placeholder for Sema's typo correction. But of course defining AST nodes
that don't live in the AST library to a certain degree isn't possible... ah
well. I knew I'd have to deal with the layering issue at some point

* The TypoExpr class should instead store a single opaque 'void*' pointer,
Post by Richard Smith
and make no attempt to deallocate or otherwise manage it.
* Sema owns the pointee and is responsible for deleting it (you can delete
it and zero out the pointer on the TypoExpr at the point when you handle
the TypoExpr)
Alternatively, since you need Sema to track the live TypoExprs anyway, you
could have Sema maintain a map from TypoExpr* to whatever data you want to
store for the TypoExprs, and have the AST node be essentially stateless.
I think I'll go with this approach since a TypoExpr node is just a
placeholder for internal use by Sema. For the mapping, do you think it
would be better to move the recovery callback and diagnostic generator into
the TypoCorrectionConsumer so that the map is simply from a TypoExpr to a
TypoCorrectionConsumer, or to place the three unique_ptrs in a tuple that
would go into the map, or to wrap the three unique_ptrs in a struct that is
stored in the map?

Incidentally, I wonder whether it'd be useful for TypoExpr to store the
Post by Richard Smith
typoed identifier, maybe as an IdentifierInfo*? (This'd allow you to
produce something more useful from the statement printer and ast dumper,
and we really don't care much how big a TypoExpr is, since we'll --
presumably -- never have many of them.) This might also be useful for
clients that want to turn off typo correction but keep the TypoExprs in the
AST.
As the typo correction is currently structured, a TypoExpr would never be
generated when typo correction is turned off since the TypoExpr is only for
Sema to know when and where to go back to perform typo correction when it
wasn't immediately performed. I'm also not sure how useful it would be to
have meaningful output from the statement printer and AST dumper since the
TypoExpr shouldn't be kept around that long (it should be gone by the time
the full statement has finished being parsed and analysed--I cannot think
of any cases where that wouldn't be true that wouldn't also be a bug; after
all, until the TypoExpr is handled Sema won't know if the error it
represents is recoverable or what the recovered AST should be). If/once
there's a good use case for having the identifier-to-be-corrected stored in
the TypoExpr, it'll be simple enough to add since it is available when the
TypoExpr is created and is also already being stored in the
TypoCorrectionConsumer.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/6c632db3/attachment.html>
Richard Smith
2014-07-25 22:29:21 UTC
Permalink
Post by Kaelyn Takata
On Fri, Jul 25, 2014 at 1:57 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
Post by Kaelyn Takata
---
include/clang/AST/DataRecursiveASTVisitor.h | 1 +
include/clang/AST/Expr.h | 20 ++++++++++++++++++++
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 | 16 ++++++++++++++++
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 +
19 files changed, 80 insertions(+), 1 deletion(-)
The AST library is supposed to be self-contained; it's a layering
violation to put TypoExpr member definitions into Sema. Conversely, it'd be
a layering violation to put them into AST, since they currently delete Sema
I'd have actually preferred to keep TypoExpr out of the AST library
altogether since it is a synthetic node generated by Sema for use by Sema
and really shouldn't ever be seen/touched outside of Sema, as it is simply
a placeholder for Sema's typo correction. But of course defining AST nodes
that don't live in the AST library to a certain degree isn't possible... ah
well. I knew I'd have to deal with the layering issue at some point
* The TypoExpr class should instead store a single opaque 'void*' pointer,
Post by Richard Smith
and make no attempt to deallocate or otherwise manage it.
* Sema owns the pointee and is responsible for deleting it (you can
delete it and zero out the pointer on the TypoExpr at the point when you
handle the TypoExpr)
Alternatively, since you need Sema to track the live TypoExprs anyway,
you could have Sema maintain a map from TypoExpr* to whatever data you want
to store for the TypoExprs, and have the AST node be essentially stateless.
I think I'll go with this approach since a TypoExpr node is just a
placeholder for internal use by Sema. For the mapping, do you think it
would be better to move the recovery callback and diagnostic generator into
the TypoCorrectionConsumer so that the map is simply from a TypoExpr to a
TypoCorrectionConsumer, or to place the three unique_ptrs in a tuple that
would go into the map, or to wrap the three unique_ptrs in a struct that is
stored in the map?
Since the map will be internal to Sema, I think you should go with whatever
option makes the Sema implementation most convenient. Since we use
TypoCorrectionConsumer for the non-delayed case, keeping the other parts
separate seems likely to be best.
Post by Kaelyn Takata
Incidentally, I wonder whether it'd be useful for TypoExpr to store the
Post by Richard Smith
typoed identifier, maybe as an IdentifierInfo*? (This'd allow you to
produce something more useful from the statement printer and ast dumper,
and we really don't care much how big a TypoExpr is, since we'll --
presumably -- never have many of them.) This might also be useful for
clients that want to turn off typo correction but keep the TypoExprs in the
AST.
As the typo correction is currently structured, a TypoExpr would never be
generated when typo correction is turned off since the TypoExpr is only for
Sema to know when and where to go back to perform typo correction when it
wasn't immediately performed. I'm also not sure how useful it would be to
have meaningful output from the statement printer and AST dumper since the
TypoExpr shouldn't be kept around that long (it should be gone by the time
the full statement has finished being parsed and analysed--I cannot think
of any cases where that wouldn't be true that wouldn't also be a bug; after
all, until the TypoExpr is handled Sema won't know if the error it
represents is recoverable or what the recovered AST should be). If/once
there's a good use case for having the identifier-to-be-corrected stored in
the TypoExpr, it'll be simple enough to add since it is available when the
TypoExpr is created and is also already being stored in the
TypoCorrectionConsumer.
OK, I'm happy keeping it empty for now. I asked because we have several
clang-as-a-library users who've asked for both
a) parse what you can, and build an approximate AST for what you can't, and
b) tell me where the typos are, but don't actually correct them
... and it seems that we could improve the situation in both cases if we
had a mode to create TypoExprs but not act on them. Let's keep that as a
separate discussion to be had once this work lands, though.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/40b9cc12/attachment.html>
Kaelyn Takata
2014-07-25 23:39:19 UTC
Permalink
On Fri, Jul 25, 2014 at 3:29 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
Post by Kaelyn Takata
On Fri, Jul 25, 2014 at 1:57 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
Post by Kaelyn Takata
---
include/clang/AST/DataRecursiveASTVisitor.h | 1 +
include/clang/AST/Expr.h | 20 ++++++++++++++++++++
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 | 16 ++++++++++++++++
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 +
19 files changed, 80 insertions(+), 1 deletion(-)
The AST library is supposed to be self-contained; it's a layering
violation to put TypoExpr member definitions into Sema. Conversely, it'd be
a layering violation to put them into AST, since they currently delete Sema
I'd have actually preferred to keep TypoExpr out of the AST library
altogether since it is a synthetic node generated by Sema for use by Sema
and really shouldn't ever be seen/touched outside of Sema, as it is simply
a placeholder for Sema's typo correction. But of course defining AST nodes
that don't live in the AST library to a certain degree isn't possible... ah
well. I knew I'd have to deal with the layering issue at some point
* The TypoExpr class should instead store a single opaque 'void*'
Post by Richard Smith
pointer, and make no attempt to deallocate or otherwise manage it.
* Sema owns the pointee and is responsible for deleting it (you can
delete it and zero out the pointer on the TypoExpr at the point when you
handle the TypoExpr)
Alternatively, since you need Sema to track the live TypoExprs anyway,
you could have Sema maintain a map from TypoExpr* to whatever data you want
to store for the TypoExprs, and have the AST node be essentially stateless.
I think I'll go with this approach since a TypoExpr node is just a
placeholder for internal use by Sema. For the mapping, do you think it
would be better to move the recovery callback and diagnostic generator into
the TypoCorrectionConsumer so that the map is simply from a TypoExpr to a
TypoCorrectionConsumer, or to place the three unique_ptrs in a tuple that
would go into the map, or to wrap the three unique_ptrs in a struct that is
stored in the map?
Since the map will be internal to Sema, I think you should go with
whatever option makes the Sema implementation most convenient. Since we use
TypoCorrectionConsumer for the non-delayed case, keeping the other parts
separate seems likely to be best.
Post by Kaelyn Takata
Incidentally, I wonder whether it'd be useful for TypoExpr to store the
Post by Richard Smith
typoed identifier, maybe as an IdentifierInfo*? (This'd allow you to
produce something more useful from the statement printer and ast dumper,
and we really don't care much how big a TypoExpr is, since we'll --
presumably -- never have many of them.) This might also be useful for
clients that want to turn off typo correction but keep the TypoExprs in the
AST.
As the typo correction is currently structured, a TypoExpr would never be
generated when typo correction is turned off since the TypoExpr is only for
Sema to know when and where to go back to perform typo correction when it
wasn't immediately performed. I'm also not sure how useful it would be to
have meaningful output from the statement printer and AST dumper since the
TypoExpr shouldn't be kept around that long (it should be gone by the time
the full statement has finished being parsed and analysed--I cannot think
of any cases where that wouldn't be true that wouldn't also be a bug; after
all, until the TypoExpr is handled Sema won't know if the error it
represents is recoverable or what the recovered AST should be). If/once
there's a good use case for having the identifier-to-be-corrected stored in
the TypoExpr, it'll be simple enough to add since it is available when the
TypoExpr is created and is also already being stored in the
TypoCorrectionConsumer.
OK, I'm happy keeping it empty for now. I asked because we have several
clang-as-a-library users who've asked for both
a) parse what you can, and build an approximate AST for what you can't, and
b) tell me where the typos are, but don't actually correct them
... and it seems that we could improve the situation in both cases if we
had a mode to create TypoExprs but not act on them. Let's keep that as a
separate discussion to be had once this work lands, though.
Makes sense. And I agree that those cases would be an extension of the
delayed typo correction facility once it exists.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/c740f737/attachment-0001.html>
Kaelyn Takata
2014-07-14 23:55:35 UTC
Permalink
---
include/clang/Parse/Parser.h | 5 ++--
include/clang/Sema/Sema.h | 27 +++++++++----------
include/clang/Sema/SemaInternal.h | 18 ++++++-------
lib/Parse/ParseExpr.cpp | 8 +++---
lib/Parse/ParseStmt.cpp | 6 ++---
lib/Parse/ParseTentative.cpp | 8 +++---
lib/Parse/Parser.cpp | 8 +++---
lib/Sema/SemaCXXScopeSpec.cpp | 9 +++----
lib/Sema/SemaDecl.cpp | 57 +++++++++++++++++++--------------------
lib/Sema/SemaDeclCXX.cpp | 27 ++++++++++---------
lib/Sema/SemaDeclObjC.cpp | 21 +++++++--------
lib/Sema/SemaExpr.cpp | 37 ++++++++++++-------------
lib/Sema/SemaExprMember.cpp | 15 +++++------
lib/Sema/SemaExprObjC.cpp | 18 ++++++-------
lib/Sema/SemaInit.cpp | 4 +--
lib/Sema/SemaLambda.cpp | 4 +--
lib/Sema/SemaLookup.cpp | 27 +++++++++++--------
lib/Sema/SemaOpenMP.cpp | 7 +++--
lib/Sema/SemaOverload.cpp | 23 +++++++++-------
lib/Sema/SemaTemplate.cpp | 17 ++++++------
lib/Sema/SemaTemplateVariadic.cpp | 8 +++---
21 files changed, 179 insertions(+), 175 deletions(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0003-Pass-around-CorrectionCandidateCallbacks-as-uniqu.patch
Type: text/x-patch
Size: 43804 bytes
Desc: not available
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140714/0dfcde5a/attachment-0001.bin>
Richard Smith
2014-07-25 20:58:53 UTC
Permalink
Looks fine as a mechanical change; if the final approach to ownership needs
this, then LGTM.
Post by Kaelyn Takata
---
include/clang/Parse/Parser.h | 5 ++--
include/clang/Sema/Sema.h | 27 +++++++++----------
include/clang/Sema/SemaInternal.h | 18 ++++++-------
lib/Parse/ParseExpr.cpp | 8 +++---
lib/Parse/ParseStmt.cpp | 6 ++---
lib/Parse/ParseTentative.cpp | 8 +++---
lib/Parse/Parser.cpp | 8 +++---
lib/Sema/SemaCXXScopeSpec.cpp | 9 +++----
lib/Sema/SemaDecl.cpp | 57
+++++++++++++++++++--------------------
lib/Sema/SemaDeclCXX.cpp | 27 ++++++++++---------
lib/Sema/SemaDeclObjC.cpp | 21 +++++++--------
lib/Sema/SemaExpr.cpp | 37 ++++++++++++-------------
lib/Sema/SemaExprMember.cpp | 15 +++++------
lib/Sema/SemaExprObjC.cpp | 18 ++++++-------
lib/Sema/SemaInit.cpp | 4 +--
lib/Sema/SemaLambda.cpp | 4 +--
lib/Sema/SemaLookup.cpp | 27 +++++++++++--------
lib/Sema/SemaOpenMP.cpp | 7 +++--
lib/Sema/SemaOverload.cpp | 23 +++++++++-------
lib/Sema/SemaTemplate.cpp | 17 ++++++------
lib/Sema/SemaTemplateVariadic.cpp | 8 +++---
21 files changed, 179 insertions(+), 175 deletions(-)
_______________________________________________
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.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/9714cddd/attachment.html>
Kaelyn Takata
2014-07-25 21:44:25 UTC
Permalink
Yup, this is needed because the TypoExpr generally isn't transformed until
well after the function which provided an appropriate callback would have
gone out of scope, and the TypoCorrectionConsumer isn't called upon to
provide correction candidates that pass any validation the callback
performs until the TypoExpr is transformed.


On Fri, Jul 25, 2014 at 1:58 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
Looks fine as a mechanical change; if the final approach to ownership
needs this, then LGTM.
Post by Kaelyn Takata
---
include/clang/Parse/Parser.h | 5 ++--
include/clang/Sema/Sema.h | 27 +++++++++----------
include/clang/Sema/SemaInternal.h | 18 ++++++-------
lib/Parse/ParseExpr.cpp | 8 +++---
lib/Parse/ParseStmt.cpp | 6 ++---
lib/Parse/ParseTentative.cpp | 8 +++---
lib/Parse/Parser.cpp | 8 +++---
lib/Sema/SemaCXXScopeSpec.cpp | 9 +++----
lib/Sema/SemaDecl.cpp | 57
+++++++++++++++++++--------------------
lib/Sema/SemaDeclCXX.cpp | 27 ++++++++++---------
lib/Sema/SemaDeclObjC.cpp | 21 +++++++--------
lib/Sema/SemaExpr.cpp | 37 ++++++++++++-------------
lib/Sema/SemaExprMember.cpp | 15 +++++------
lib/Sema/SemaExprObjC.cpp | 18 ++++++-------
lib/Sema/SemaInit.cpp | 4 +--
lib/Sema/SemaLambda.cpp | 4 +--
lib/Sema/SemaLookup.cpp | 27 +++++++++++--------
lib/Sema/SemaOpenMP.cpp | 7 +++--
lib/Sema/SemaOverload.cpp | 23 +++++++++-------
lib/Sema/SemaTemplate.cpp | 17 ++++++------
lib/Sema/SemaTemplateVariadic.cpp | 8 +++---
21 files changed, 179 insertions(+), 175 deletions(-)
_______________________________________________
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.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/cdf21d32/attachment.html>
Kaelyn Takata
2014-07-14 23:55:36 UTC
Permalink
Two additional methods are provided: one to return the current
correction (the last correction returned by getNextCorrection), and one
to "reset" the state so that getNextCorrection will return the previous
corrections before returning any new corrections.

Also ensure that all TypoCorrections have valid source ranges.
---
include/clang/Sema/SemaInternal.h | 31 ++++++++++++++++++++++++++-----
include/clang/Sema/TypoCorrection.h | 2 +-
lib/Sema/SemaLookup.cpp | 16 ++++++++++++----
3 files changed, 39 insertions(+), 10 deletions(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0004-Have-TypoCorrectionConsumer-remember-the-TypoCorr.patch
Type: text/x-patch
Size: 4948 bytes
Desc: not available
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140714/d721cac8/attachment-0001.bin>
Richard Smith
2014-07-25 21:01:18 UTC
Permalink
+ TypoCorrection &getNextCorrection();
+
+ /// \brief Get the last correction returned by getNextCorrection().
+ TypoCorrection &getCurrentCorrection() {

Since the TypoCorrections are now intended to be reused, it would seem wise
to return them either by value or by const reference.
Post by Kaelyn Takata
Two additional methods are provided: one to return the current
correction (the last correction returned by getNextCorrection), and one
to "reset" the state so that getNextCorrection will return the previous
corrections before returning any new corrections.
Also ensure that all TypoCorrections have valid source ranges.
---
include/clang/Sema/SemaInternal.h | 31 ++++++++++++++++++++++++++-----
include/clang/Sema/TypoCorrection.h | 2 +-
lib/Sema/SemaLookup.cpp | 16 ++++++++++++----
3 files changed, 39 insertions(+), 10 deletions(-)
_______________________________________________
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.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/828b77a2/attachment.html>
Kaelyn Takata
2014-07-25 21:39:27 UTC
Permalink
On Fri, Jul 25, 2014 at 2:01 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
+ TypoCorrection &getNextCorrection();
+
+ /// \brief Get the last correction returned by getNextCorrection().
+ TypoCorrection &getCurrentCorrection() {
Since the TypoCorrections are now intended to be reused, it would seem
wise to return them either by value or by const reference.
They originally were returned by value, but there is a case where the set
of decls associated with the TypoCorrection needs to be modified for the
proper diagnostics to be emitted: specifically by the overload resolution
in the MemberExprTypoRecovery callback introduced in patch #9. Making the
return value be a non-const reference had been a separate patch that I
folded into the others to avoid unnecessary churn. I agree it would be
nicer to return them by value or const reference, but doing so greatly
complicates the recovery callback's ability to select the right method from
a group of methods with the same name in a way that will be visible to
TransformTypos::Transform when it comes time to emit diagnostics
(particularly without introducing any additional AST<->Sema layering
violations).
Post by Richard Smith
Post by Kaelyn Takata
Two additional methods are provided: one to return the current
correction (the last correction returned by getNextCorrection), and one
to "reset" the state so that getNextCorrection will return the previous
corrections before returning any new corrections.
Also ensure that all TypoCorrections have valid source ranges.
---
include/clang/Sema/SemaInternal.h | 31 ++++++++++++++++++++++++++-----
include/clang/Sema/TypoCorrection.h | 2 +-
lib/Sema/SemaLookup.cpp | 16 ++++++++++++----
3 files changed, 39 insertions(+), 10 deletions(-)
_______________________________________________
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.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/bcf59621/attachment.html>
Richard Smith
2014-07-25 22:30:42 UTC
Permalink
Post by Kaelyn Takata
On Fri, Jul 25, 2014 at 2:01 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
+ TypoCorrection &getNextCorrection();
+
+ /// \brief Get the last correction returned by getNextCorrection().
+ TypoCorrection &getCurrentCorrection() {
Since the TypoCorrections are now intended to be reused, it would seem
wise to return them either by value or by const reference.
They originally were returned by value, but there is a case where the set
of decls associated with the TypoCorrection needs to be modified for the
proper diagnostics to be emitted: specifically by the overload resolution
in the MemberExprTypoRecovery callback introduced in patch #9. Making the
return value be a non-const reference had been a separate patch that I
folded into the others to avoid unnecessary churn. I agree it would be
nicer to return them by value or const reference, but doing so greatly
complicates the recovery callback's ability to select the right method from
a group of methods with the same name in a way that will be visible to
TransformTypos::Transform when it comes time to emit diagnostics
(particularly without introducing any additional AST<->Sema layering
violations).
OK. My main concern was that someone might be accidentally modifying a
TypoCorrection and then disrupting other people who rewind the typo
correction stream and look at the same typo again. Perhaps that's not a
concern.
Post by Kaelyn Takata
Post by Richard Smith
Post by Kaelyn Takata
Two additional methods are provided: one to return the current
correction (the last correction returned by getNextCorrection), and one
to "reset" the state so that getNextCorrection will return the previous
corrections before returning any new corrections.
Also ensure that all TypoCorrections have valid source ranges.
---
include/clang/Sema/SemaInternal.h | 31 ++++++++++++++++++++++++++-----
include/clang/Sema/TypoCorrection.h | 2 +-
lib/Sema/SemaLookup.cpp | 16 ++++++++++++----
3 files changed, 39 insertions(+), 10 deletions(-)
_______________________________________________
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.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/ec09b7be/attachment.html>
Kaelyn Takata
2014-07-14 23:55:37 UTC
Permalink
The the typo count is propagated up the stack of
ExpressionEvaluationContextRecords when one is popped off of to
avoid accidentally dropping TypoExprs on the floor. For example,
the attempted correction of g() in test/CXX/class/class.mem/p5-0x.cpp
happens with an ExpressionEvaluationContextRecord that is popped off
the stack prior to ActOnFinishFullExpr being called and the tree
transform for TypoExprs being run.
---
include/clang/Sema/Sema.h | 25 ++++
include/clang/Sema/SemaInternal.h | 15 +-
lib/Sema/SemaExpr.cpp | 7 +
lib/Sema/SemaExprCXX.cpp | 109 ++++++++++++++
lib/Sema/SemaLookup.cpp | 291 ++++++++++++++++++++++++--------------
5 files changed, 340 insertions(+), 107 deletions(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0005-Start-adding-the-infrastructure-for-handling-Typo.patch
Type: text/x-patch
Size: 30161 bytes
Desc: not available
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140714/38e203c8/attachment-0001.bin>
Richard Smith
2014-07-25 21:27:36 UTC
Permalink
Post by Kaelyn Takata
The the typo count is propagated up the stack of
ExpressionEvaluationContextRecords when one is popped off of to
avoid accidentally dropping TypoExprs on the floor. For example,
the attempted correction of g() in test/CXX/class/class.mem/p5-0x.cpp
happens with an ExpressionEvaluationContextRecord that is popped off
the stack prior to ActOnFinishFullExpr being called and the tree
transform for TypoExprs being run.
FWIW, that case sounds like a bug: finalizing the full-expression should
happen within the relevant ExpressionEvaluationContext.

Is a simple count of TypoExprs sufficient here? If we leave the local
expression without diagnosing a typo (or if our RecursiveASTVisitor fails
to diagnose it), I think we still need to track it so that we can produce
some kind of diagnostic for it. In particular:

+ assert(NumTypos == 0 && "There are outstanding typos after popping the
"
+ "last ExpressionEvaluationContextRecord");

... I think there are probably cases where this assertion can fire, and the
right thing to do seems to be to issue a basic 'unknown identifier'
diagnostic for any remaining typos.


+ llvm::SmallVector<Expr *, 4> ExprStack;

This seems rather small; I expect we'd walk through more Exprs than this.
But... you don't actually use this. Can you remove it?


+ // If corrections for the first TypoExpr have been exhausted, retry
them
+ // against the next combination of substitutions for all of the other
+ // TypoExprs.
+ if (FirstTypoExpr->Consumer->finished()) {
+ unsigned numFinished = 1;
+ for (auto TE : TypoExprs) {
+ if (TE != FirstTypoExpr) {
+ TransformCache.erase(TE);
+ if (TE->Consumer->finished()) {
+ ++numFinished;
+ TE->Consumer->resetCorrectionStream();
+ } else {
+ break;
+ }
+ }
+ }
+ FirstTypoExpr->Consumer->resetCorrectionStream();
+ if (numFinished >= TypoExprs.size())
+ break;
+ }

It looks like the net result of this is that you'll try:

* Typo 1 correction 1, typo 2 correction 1, typo 3 correction 1
* Typo 1 correction 2, typo 2 correction 1, typo 3 correction 1
* ...
* Typo 1 correction 1, typo 2 correction 2, typo 3 correction 2
* Typo 1 correction 2, typo 2 correction 2, typo 3 correction 2
* ...

... and you'll never try typo 2 correction 2 with typo 3 correction 1.
(Also, if typo 2 has 2 corrections and typo 3 has 3, you'll never consider
the third correction for typo 3).

I think we need to decide:
* how many different combinations of typo corrections we're prepared to
try, and
* which order to search them in

Due to the tree structure of the AST, we should be able to do better than
trying all combinations; for instance, given:

f(typo1, g(typo2, typo3))

... we can figure out which combinations work for the call to g, and only
try those combinations in the call to f.


@@ -5964,6 +6063,16 @@ ExprResult Sema::ActOnFinishFullExpr(Expr *FE,
SourceLocation CC,
return ExprError();
}

+ if (ExprEvalContexts.back().NumTypos &&
+ (FullExpr.get()->isTypeDependent() ||
+ FullExpr.get()->isValueDependent() ||
+ FullExpr.get()->isInstantiationDependent())) {
+ FullExpr = TransformTypos(*this).Transform(FullExpr.get());
+ ExprEvalContexts.back().NumTypos = 0;
+ if (FullExpr.isInvalid())
+ return ExprError();
+ }

This is assuming that any call to TransformTypos::Transform will remove all
TypoExprs within the entire ExpressionEvaluationContext. I am not sure that
this will necessarily be true in general. It'd be more robust for
TransformTypos to specify which TypoExprs it transformed, and to remove
those from the ExpressionEvaluationContext or from a list of currently
undiagnosed TypoExprs.


+/// \returns a new \c TypoExpr that will later be replaced in the AST with
an
+/// Expr representing the result of performing typo correction.
+TypoExpr *Sema::CorrectTypoDelayed(

It would be useful to document that the caller still needs to produce a
diagnostic if this returns nullptr.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/3202d873/attachment-0001.html>
Kaelyn Takata
2014-07-29 17:51:59 UTC
Permalink
On Fri, Jul 25, 2014 at 2:27 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
Post by Kaelyn Takata
The the typo count is propagated up the stack of
ExpressionEvaluationContextRecords when one is popped off of to
avoid accidentally dropping TypoExprs on the floor. For example,
the attempted correction of g() in test/CXX/class/class.mem/p5-0x.cpp
happens with an ExpressionEvaluationContextRecord that is popped off
the stack prior to ActOnFinishFullExpr being called and the tree
transform for TypoExprs being run.
FWIW, that case sounds like a bug: finalizing the full-expression should
happen within the relevant ExpressionEvaluationContext.
I don't know enough about what the intended mechanics/interactions of these
within clang is intended to be to say for sure, but my recent experiences
with them suggests it isn't a bug. As I understand it, the declaration of b
and its initialization based on the call to g() is the full expression
within an expression evaluation context, but the the initializer (the call
to g()) would not and should not include the declaration of b and so is
given its own (sub-)expression evaluation context doesn't span the full
expression or include the declaration of b.

Is a simple count of TypoExprs sufficient here? If we leave the local
Post by Richard Smith
expression without diagnosing a typo (or if our RecursiveASTVisitor fails
to diagnose it), I think we still need to track it so that we can produce
+ assert(NumTypos == 0 && "There are outstanding typos after popping
the "
+ "last ExpressionEvaluationContextRecord");
... I think there are probably cases where this assertion can fire, and
the right thing to do seems to be to issue a basic 'unknown identifier'
diagnostic for any remaining typos.
IIRC, the assertion has actually been useful when hooking up the delayed
typo correction to a primary caller of DiagnoseEmptyLookup in that it has
helped me find cases where ActOnFinishFullExpr aren't ever called. However,
now that Sema will be keeping track of all of the undiagnosed TypoExprs and
their associated state to remove the layering violations in TypoExpr, this
tracking of whether an Expr in a given evaluation context might need the
tree transform run on it needs to be reworked.

+ llvm::SmallVector<Expr *, 4> ExprStack;
Post by Richard Smith
This seems rather small; I expect we'd walk through more Exprs than this.
But... you don't actually use this. Can you remove it?
It is used in patch #8 for passing the parent Expr of the TypoExpr to the
recovery callback, so that e.g. the number of arguments the TypoExpr is
being called with can be known.
Post by Richard Smith
+ // If corrections for the first TypoExpr have been exhausted, retry
them
+ // against the next combination of substitutions for all of the other
+ // TypoExprs.
+ if (FirstTypoExpr->Consumer->finished()) {
+ unsigned numFinished = 1;
+ for (auto TE : TypoExprs) {
+ if (TE != FirstTypoExpr) {
+ TransformCache.erase(TE);
+ if (TE->Consumer->finished()) {
+ ++numFinished;
+ TE->Consumer->resetCorrectionStream();
+ } else {
+ break;
+ }
+ }
+ }
+ FirstTypoExpr->Consumer->resetCorrectionStream();
+ if (numFinished >= TypoExprs.size())
+ break;
+ }
* Typo 1 correction 1, typo 2 correction 1, typo 3 correction 1
* Typo 1 correction 2, typo 2 correction 1, typo 3 correction 1
* ...
* Typo 1 correction 1, typo 2 correction 2, typo 3 correction 2
* Typo 1 correction 2, typo 2 correction 2, typo 3 correction 2
* ...
... and you'll never try typo 2 correction 2 with typo 3 correction 1.
(Also, if typo 2 has 2 corrections and typo 3 has 3, you'll never consider
the third correction for typo 3).
I believe you are forgetting to account for the TransformCache which keeps
the cached TypoExprs from advancing Unless I screwed up the logic, the net
result should be e.g. (assuming typo 1 has 3 candidates, typo 2 has 2, and
typo 3 has 4):

* Typo 1 correction 1, typo 2 correction 1, typo 3 correction 1
* Typo 1 correction 2, typo 2 correction 1, typo 3 correction 1
* Typo 1 correction 3, typo 2 correction 1, typo 3 correction 1
* Typo 1 correction 1, typo 2 correction 2, typo 3 correction 1
* Typo 1 correction 2, typo 2 correction 2, typo 3 correction 1
* Typo 1 correction 3, typo 2 correction 2, typo 3 correction 1
* Typo 1 correction 1, typo 2 correction 1, typo 3 correction 2
* Typo 1 correction 2, typo 2 correction 1, typo 3 correction 2
* Typo 1 correction 3, typo 2 correction 1, typo 3 correction 2
* Typo 1 correction 1, typo 2 correction 2, typo 3 correction 2
* Typo 1 correction 2, typo 2 correction 2, typo 3 correction 2
* Typo 1 correction 3, typo 2 correction 2, typo 3 correction 2
* Typo 1 correction 1, typo 2 correction 1, typo 3 correction 3
* Typo 1 correction 2, typo 2 correction 1, typo 3 correction 3
* Typo 1 correction 3, typo 2 correction 1, typo 3 correction 3
* Typo 1 correction 1, typo 2 correction 2, typo 3 correction 3
* Typo 1 correction 2, typo 2 correction 2, typo 3 correction 3
* Typo 1 correction 3, typo 2 correction 2, typo 3 correction 3
* Typo 1 correction 1, typo 2 correction 1, typo 3 correction 4
* Typo 1 correction 2, typo 2 correction 1, typo 3 correction 4
* Typo 1 correction 3, typo 2 correction 1, typo 3 correction 4
* Typo 1 correction 1, typo 2 correction 2, typo 3 correction 4
* Typo 1 correction 2, typo 2 correction 2, typo 3 correction 4
* Typo 1 correction 3, typo 2 correction 2, typo 3 correction 4

The code:
1) notes the first TypoExpr encountered
2) for each TypoExpr encountered, if is the first TypoExpr or it isn't in
the cache, goes through the standard logic for choosing a new candidate and
caches the result, otherwise returns the cached result
3) then once the first TypoExpr is finished, resets it and removes the
next TypoExpr from the cache so a new result will be generated for that
TypoExpr, and if that TypoExpr is also finished resets it and removes the
following TypoExpr from the cache, repeating until a TypoExpr that wasn't
finished is removed from the cache or all TypoExprs were seen to be
finished.

Since this seems to not be as clear/obvious as I thought, shall I add a
brief explanation about how the retrying interacts with the caching to the
comment on the loop?
Post by Richard Smith
* how many different combinations of typo corrections we're prepared to
try, and
* which order to search them in
Right now the code tries all possible combinations that passed validation,
in a depth-first manner.
Post by Richard Smith
Due to the tree structure of the AST, we should be able to do better than
f(typo1, g(typo2, typo3))
... we can figure out which combinations work for the call to g, and only
try those combinations in the call to f.
Unless the tree transform code doesn't short-circuit the transform once an
ExprError is encountered, the existing code already does this to a large
degree (and only for the subset of possible corrections that CorrectTypo
would have returned anyway). Given that having more than one or two typos
in a single full-expression seems to be quite uncommon, I suspect that
trying to be any more clever about which sub-combinations do or don't work
to avoid trying all combinations in this error path will not yield
sufficient gains to warrant the additional design effort and code
complexity.
Post by Richard Smith
@@ -5964,6 +6063,16 @@ ExprResult Sema::ActOnFinishFullExpr(Expr *FE,
SourceLocation CC,
return ExprError();
}
+ if (ExprEvalContexts.back().NumTypos &&
+ (FullExpr.get()->isTypeDependent() ||
+ FullExpr.get()->isValueDependent() ||
+ FullExpr.get()->isInstantiationDependent())) {
+ FullExpr = TransformTypos(*this).Transform(FullExpr.get());
+ ExprEvalContexts.back().NumTypos = 0;
+ if (FullExpr.isInvalid())
+ return ExprError();
+ }
This is assuming that any call to TransformTypos::Transform will remove
all TypoExprs within the entire ExpressionEvaluationContext. I am not sure
that this will necessarily be true in general. It'd be more robust for
TransformTypos to specify which TypoExprs it transformed, and to remove
those from the ExpressionEvaluationContext or from a list of currently
undiagnosed TypoExprs.
As mentioned, the count tracking needs to be reworked now that Sema will be
maintaining the set of undiagnosed TypoExprs anyway, and what I have in
mind for fixing it will also solve this problem (I knew the current count
tracking was quite fragile, but it was sufficient for hooking the delayed
typo correction to member lookup and I didn't want to put a lot of effort
into wiring up precise tracking of the count when large parts of the code
may be changing anyway--and with Sema now maintaining a map of undiagnosed
TypoExprs to their associated Sema-specific state, I'm glad I hadn't tried
to add a bunch of fancy typo count tracking).
Post by Richard Smith
+/// \returns a new \c TypoExpr that will later be replaced in the AST
with an
+/// Expr representing the result of performing typo correction.
+TypoExpr *Sema::CorrectTypoDelayed(
It would be useful to document that the caller still needs to produce a
diagnostic if this returns nullptr.
Actually, I think it may make more sense to invoke the
TypoDiagnosticGenerator with an empty correction before returning nullptr
instead of foisting that responsibility on the caller.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140729/b70c7e6a/attachment.html>
Kaelyn Takata
2014-07-14 23:55:38 UTC
Permalink
Also be more proactive about checking a correction's visibility so that
a correction requiring a module import can be distinguished from the
original typo even if it looks identical. Otherwise the correction will
be excluded and the diagnostic about needing the module import won't be
emitted.

Note that no change was made to checkCorrectionVisibility other than
moving where it is at in SemaLookup.cpp.
---
include/clang/Sema/TypoCorrection.h | 24 +++++++--
lib/Sema/SemaLookup.cpp | 100 +++++++++++++++++++-----------------
2 files changed, 74 insertions(+), 50 deletions(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0006-Add-simple-way-for-a-CorrectionCandidateCallback-.patch
Type: text/x-patch
Size: 7359 bytes
Desc: not available
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140714/f1d2ed2b/attachment-0001.bin>
Richard Smith
2014-07-25 21:52:04 UTC
Permalink
Post by Kaelyn Takata
Also be more proactive about checking a correction's visibility so that
a correction requiring a module import can be distinguished from the
original typo even if it looks identical. Otherwise the correction will
be excluded and the diagnostic about needing the module import won't be
emitted.
Note that no change was made to checkCorrectionVisibility other than
moving where it is at in SemaLookup.cpp.
Perhaps I'm missing the trick here (since there are no tests as part of
this change, it's not easy to see exactly how this pans out), but I don't
see how we're producing the missing import diagnostic along the delayed
typo correction codepath in a way that doesn't cause typo correction to
fail.

That said, I don't think we need to do so (and perhaps this is how this
already works out): if we can resolve the "typo" with a module import, we
can correct it on the fly and never build a TypoExpr. If we actually need
to correct the typo, then we never need to add a module import (because we
never consider typo-correcting to a non-visible name).


+ if (MatchesTypo(candidate))
+ return false;

When does this happen?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/84a950b5/attachment.html>
Kaelyn Takata
2014-07-25 23:36:07 UTC
Permalink
On Fri, Jul 25, 2014 at 2:52 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
Post by Kaelyn Takata
Also be more proactive about checking a correction's visibility so that
a correction requiring a module import can be distinguished from the
original typo even if it looks identical. Otherwise the correction will
be excluded and the diagnostic about needing the module import won't be
emitted.
Note that no change was made to checkCorrectionVisibility other than
moving where it is at in SemaLookup.cpp.
Perhaps I'm missing the trick here (since there are no tests as part of
this change, it's not easy to see exactly how this pans out), but I don't
see how we're producing the missing import diagnostic along the delayed
typo correction codepath in a way that doesn't cause typo correction to
fail.
That said, I don't think we need to do so (and perhaps this is how this
already works out): if we can resolve the "typo" with a module import, we
can correct it on the fly and never build a TypoExpr. If we actually need
to correct the typo, then we never need to add a module import (because we
never consider typo-correcting to a non-visible name).
Well this does affect both the immediate and delayed typo correction
codepaths. I think the only "trick" that you are missing is that if there
is an identifier that is a typo being corrected and there is a
textually-identical identifier that is being imported from a module, they
are different just as if there is a textually-identical identifier that has
a valid decl (i.e. the resolved/imported version of the identifier doesn't
work in the current context and so led to typo correction being invoked).
Also, once I started hooking up CorrectTypoDelayed to DiagnoseEmptyLookup,
not checking candidate.requiresImport() in
CorrectionCandidateCallback::MatchesTypo breaks three of the modules tests:

********************
FAIL: Clang :: Modules/auto-module-import.m (3675 of 7523)
******************** TEST 'Clang :: Modules/auto-module-import.m' FAILED
********************
Script:
--
rm -rf
/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/auto-module-import.m.tmp
/mnt/storage/rikka/llvm/build/./bin/clang -cc1 -internal-isystem
/mnt/storage/rikka/llvm/build/bin/../lib/clang/3.5.0/include -Wauto-import
-fmodules-cache-path=/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/auto-module-import.m.tmp
-fmodules -F /mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m
-verify -DERRORS
/mnt/storage/rikka/llvm/build/./bin/clang -cc1 -internal-isystem
/mnt/storage/rikka/llvm/build/bin/../lib/clang/3.5.0/include -Wauto-import
-fmodules-cache-path=/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/auto-module-import.m.tmp
-fmodules -F /mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m
-verify
--
Exit Code: 1

Command Output (stderr):
--
error: 'error' diagnostics expected but not seen:
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line
30: declaration of 'sub_framework_other' must be imported from module
'DependsOnModule.SubFramework.Other'
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line
74: declaration of 'no_umbrella_B_private' must be imported from module
'NoUmbrella.Private.B_Private'
error: 'error' diagnostics seen but not expected:
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line
30: use of undeclared identifier 'sub_framework_other'
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line
49: use of undeclared identifier 'sub_framework_other'
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m Line
74: use of undeclared identifier 'no_umbrella_B_private'; did you mean
'no_umbrella_A_private'?
error: 'note' diagnostics expected but not seen:
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/DependsOnModule.framework/Frameworks/SubFramework.framework/Headers/Other.h
Line 1 (directive at
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m:31):
previous
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/NoUmbrella.framework/PrivateHeaders/B_Private.h
Line 1 (directive at
/mnt/storage/rikka/llvm/tools/clang/test/Modules/auto-module-import.m:75):
previous
error: 'note' diagnostics seen but not expected:
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/NoUmbrella.framework/PrivateHeaders/A_Private.h
Line 1: 'no_umbrella_A_private' declared here
8 errors generated.

--

********************
FAIL: Clang :: Modules/normal-module-map.cpp (3725 of 7523)
******************** TEST 'Clang :: Modules/normal-module-map.cpp' FAILED
********************
Script:
--
rm -rf
/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/normal-module-map.cpp.tmp
/mnt/storage/rikka/llvm/build/./bin/clang -cc1 -internal-isystem
/mnt/storage/rikka/llvm/build/bin/../lib/clang/3.5.0/include -x objective-c
-fmodules-cache-path=/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/normal-module-map.cpp.tmp
-fmodules -I
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/normal-module-map
/mnt/storage/rikka/llvm/tools/clang/test/Modules/normal-module-map.cpp
-verify
--
Exit Code: 1

Command Output (stderr):
--
error: 'error' diagnostics expected but not seen:
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/normal-module-map.cpp Line
26 (directive at
/mnt/storage/rikka/llvm/tools/clang/test/Modules/normal-module-map.cpp:27):
declaration of 'nested_umbrella_b' must be imported from module
'nested_umbrella.b' before it is required
error: 'error' diagnostics seen but not expected:
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/normal-module-map.cpp Line
26: use of undeclared identifier 'nested_umbrella_b'; did you mean
'nested_umbrella_a'?
error: 'note' diagnostics expected but not seen:
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/normal-module-map/nested_umbrella/b.h
Line 1 (directive at
/mnt/storage/rikka/llvm/tools/clang/test/Modules/normal-module-map.cpp:28):
previous
error: 'note' diagnostics seen but not expected:
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/normal-module-map/nested_umbrella/a.h
Line 1: 'nested_umbrella_a' declared here
4 errors generated.

--

********************
FAIL: Clang :: Modules/subframeworks.m (3759 of 7523)
******************** TEST 'Clang :: Modules/subframeworks.m' FAILED
********************
Script:
--
rm -rf
/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/subframeworks.m.tmp
/mnt/storage/rikka/llvm/build/./bin/clang -cc1 -internal-isystem
/mnt/storage/rikka/llvm/build/bin/../lib/clang/3.5.0/include -Wauto-import
-fmodules-cache-path=/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/subframeworks.m.tmp
-fmodules -F /mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs -F
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/DependsOnModule.framework/Frameworks
/mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m -verify
/mnt/storage/rikka/llvm/build/./bin/clang -cc1 -internal-isystem
/mnt/storage/rikka/llvm/build/bin/../lib/clang/3.5.0/include -x
objective-c++ -Wauto-import
-fmodules-cache-path=/mnt/storage/rikka/llvm/build/tools/clang/test/Modules/Output/subframeworks.m.tmp
-fmodules -F /mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs -F
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/DependsOnModule.framework/Frameworks
/mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m -verify
--
Exit Code: 1

Command Output (stderr):
--
error: 'error' diagnostics expected but not seen:
File /mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m
Line 8: declaration of 'sub_framework' must be imported from module
'DependsOnModule.SubFramework' before it is required
error: 'error' diagnostics seen but not expected:
File /mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m
Line 8: use of undeclared identifier 'sub_framework'
error: 'note' diagnostics expected but not seen:
File
/mnt/storage/rikka/llvm/tools/clang/test/Modules/Inputs/DependsOnModule.framework/Frameworks/SubFramework.framework/Headers/SubFramework.h
Line 2 (directive at
/mnt/storage/rikka/llvm/tools/clang/test/Modules/subframeworks.m:9):
previous
3 errors generated.

--
Post by Richard Smith
+ if (MatchesTypo(candidate))
+ return false;
When does this happen?
It doesn't; I forgot to delete this (or it snuck back in while I was
merging and shuffling around patches to prep them for review) after moving
the check into CorrectionCandidateCallback:ValidateCandidate so that
classes which override ValidateCandidate don't accidentally lose this check
(as it prevents suggesting the original typo as a correction in cases like
"int foobar = foobar;").
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/0286a9da/attachment.html>
Kaelyn Takata
2014-07-14 23:55:39 UTC
Permalink
The new flag, WantFunctionLikeCasts, covers a subset of the keywords
covered by WantTypeSpecifiers that can be used in casts that look like
function calls, e.g. "return long(5);", while excluding the keywords
like "enum" and "const" that would be included when WantTypeSpecifiers
is true but cannot be used in something that looks like a function call.
---
include/clang/Sema/TypoCorrection.h | 12 ++++++++----
lib/Sema/SemaLookup.cpp | 10 +++++++++-
2 files changed, 17 insertions(+), 5 deletions(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0007-Add-another-keyword-selection-flag-to-CorrectionC.patch
Type: text/x-patch
Size: 2922 bytes
Desc: not available
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140714/711f98a4/attachment-0001.bin>
Richard Smith
2014-07-25 21:53:02 UTC
Permalink
Post by Kaelyn Takata
The new flag, WantFunctionLikeCasts, covers a subset of the keywords
covered by WantTypeSpecifiers that can be used in casts that look like
function calls, e.g. "return long(5);", while excluding the keywords
like "enum" and "const" that would be included when WantTypeSpecifiers
is true but cannot be used in something that looks like a function call.
Can this be tested and committed independently of the other TypoExpr
changes?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/d5f3ee38/attachment.html>
Kaelyn Takata
2014-07-28 18:23:32 UTC
Permalink
On Fri, Jul 25, 2014 at 2:53 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
Post by Kaelyn Takata
The new flag, WantFunctionLikeCasts, covers a subset of the keywords
covered by WantTypeSpecifiers that can be used in casts that look like
function calls, e.g. "return long(5);", while excluding the keywords
like "enum" and "const" that would be included when WantTypeSpecifiers
is true but cannot be used in something that looks like a function call.
Can this be tested and committed independently of the other TypoExpr
changes?
Yup, I managed to come up with a test for it. :) Committed as r214109.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140728/c510b6e6/attachment-0001.html>
Kaelyn Takata
2014-07-14 23:55:40 UTC
Permalink
---
include/clang/AST/Expr.h | 5 ++++-
include/clang/Sema/Sema.h | 3 ++-
include/clang/Sema/TypoCorrection.h | 8 ++++++++
lib/Sema/SemaExprCXX.cpp | 24 ++++++++++++++++--------
lib/Sema/SemaLookup.cpp | 13 +++++++++----
5 files changed, 39 insertions(+), 14 deletions(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0008-Add-a-callback-for-recovering-using-a-typo-correc.patch
Type: text/x-patch
Size: 6229 bytes
Desc: not available
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140714/0a66783a/attachment-0001.bin>
Kaelyn Takata
2014-07-14 23:55:41 UTC
Permalink
This includes adding the new TypoExpr-based lazy typo correction to
LookupMemberExprInRecord as an alternative to the existing eager typo
correction.
---
lib/Sema/SemaExprMember.cpp | 149 +++++++++++++++++++++++++++++--
test/SemaCXX/arrow-operator.cpp | 5 +-
test/SemaCXX/typo-correction-delayed.cpp | 32 +++++++
test/SemaCXX/typo-correction-pt2.cpp | 2 +-
test/SemaCXX/typo-correction.cpp | 10 +--
5 files changed, 182 insertions(+), 16 deletions(-)
create mode 100644 test/SemaCXX/typo-correction-delayed.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0009-Wire-up-LookupMemberExpr-to-use-the-new-TypoExpr.patch
Type: text/x-patch
Size: 12683 bytes
Desc: not available
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140714/2c5e29ae/attachment.bin>
Richard Smith
2014-07-25 22:03:14 UTC
Permalink
+ if (TE && SemaRef.getLangOpts().CPlusPlus) {
+ // TODO: C cannot handle TypoExpr nodes because the C code paths do
not know
+ // what to do with dependent types e.g. on the LHS of an assigment.
+ *TE = SemaRef.CorrectTypoDelayed(

Does this recover correctly if CorrectTypoDelayed returns nullptr? I'd have
expected that you should carry on and produce an error in this case.


+ SemaRef.AddMethodCandidate(
+ DeclAccessPair::make(ND, AS_none),
BaseType.getNonReferenceType(),
+
/*MemberRef.get()->Classify(SemaRef.Context)*/Expr::Classification::makeSimpleLValue(),
Args, CandidateSet);

Commented-out code?


+ // Perform overload resolution.
+ OverloadCandidateSet::iterator Best;
+ auto result = CandidateSet.BestViableFunction(
+ SemaRef, CE->getLocStart(), Best);

Do you need to do this here? Can you instead simply build an unresolved
overload set and allow the TreeTransform to do overload resolution when it
transforms the parent, or is there some subtlety that makes this necessary?


+ something(obj.toobat, // expected-error {{did you mean 'foobar'?}}
+ obj.toobat); // expected-error {{did you mean 'toobad'?}}

I love this testcase =)
Post by Kaelyn Takata
This includes adding the new TypoExpr-based lazy typo correction to
LookupMemberExprInRecord as an alternative to the existing eager typo
correction.
---
lib/Sema/SemaExprMember.cpp | 149
+++++++++++++++++++++++++++++--
test/SemaCXX/arrow-operator.cpp | 5 +-
test/SemaCXX/typo-correction-delayed.cpp | 32 +++++++
test/SemaCXX/typo-correction-pt2.cpp | 2 +-
test/SemaCXX/typo-correction.cpp | 10 +--
5 files changed, 182 insertions(+), 16 deletions(-)
create mode 100644 test/SemaCXX/typo-correction-delayed.cpp
_______________________________________________
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.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140725/211924c4/attachment-0001.html>
Kaelyn Takata
2014-07-30 18:20:52 UTC
Permalink
On Fri, Jul 25, 2014 at 3:03 PM, Richard Smith <richard at metafoo.co.uk>
Post by Richard Smith
+ if (TE && SemaRef.getLangOpts().CPlusPlus) {
+ // TODO: C cannot handle TypoExpr nodes because the C code paths do
not know
+ // what to do with dependent types e.g. on the LHS of an assigment.
+ *TE = SemaRef.CorrectTypoDelayed(
Does this recover correctly if CorrectTypoDelayed returns nullptr? I'd
have expected that you should carry on and produce an error in this case.
Yes it does in that the behavior of LookupMemberExprInRecord when
CorrectTypoDelayed returns nullptr is identical to the behavior of when
CorrectTypo fails: LookupMemberExprInRecord returns false and
LookupMemberExpr returns an ExprResult containing a nullptr. In both cases,
the final recovery is based on the LookupResult having been cleared.

I've dropped the superfluous ?: in LookupMemberExpr and expanded the
existing comment to include a description of the error case since the
comment had just said that returning valid-but-null meant the LookupResult
was filled in.
Post by Richard Smith
+ SemaRef.AddMethodCandidate(
+ DeclAccessPair::make(ND, AS_none),
BaseType.getNonReferenceType(),
+
/*MemberRef.get()->Classify(SemaRef.Context)*/Expr::Classification::makeSimpleLValue(),
Args, CandidateSet);
Commented-out code?
Whoops! Thanks.
Post by Richard Smith
+ // Perform overload resolution.
+ OverloadCandidateSet::iterator Best;
+ auto result = CandidateSet.BestViableFunction(
+ SemaRef, CE->getLocStart(), Best);
Do you need to do this here? Can you instead simply build an unresolved
overload set and allow the TreeTransform to do overload resolution when it
transforms the parent, or is there some subtlety that makes this necessary?
Yes the resolution has to be done here. The "subtlety" is that letting the
call to BuildMemberReferenceExpr return an UnresolvedMemberExpr when the
parent expr is a CallExpr leads to incorrect diagnostic notes, e.g. from
typo-correction-pt2.cpp:

namespace PR13387 {
struct A {
void CreateFoo(float, float);
void CreateBar(float, float);
};
struct B : A {
using A::CreateFoo;
void CreateFoo(int, int); // expected-note {{'CreateFoo' declared here}}
};
void f(B &x) {
x.Createfoo(0,0); // expected-error {{no member named 'Createfoo' in
'PR13387::B'; did you mean 'CreateFoo'?}}
}
}

Without the overload resolution, the note for CreateFoo points to the wrong
CreateFoo (it ends up pointing to the first member with that name):

test/SemaCXX/typo-correction-pt2.cpp:26:8: note: 'CreateFoo' declared here
void CreateFoo(float, float);

With the overload resolution, the note points to the correct Createfoo:

test/SemaCXX/typo-correction-pt2.cpp:31:8: note: 'CreateFoo' declared here
void CreateFoo(int, int); // expected-note {{'CreateFoo' declared here}}

I've added a comment explaining why the overload resolution is being
performed here.
Post by Richard Smith
+ something(obj.toobat, // expected-error {{did you mean 'foobar'?}}
+ obj.toobat); // expected-error {{did you mean 'toobad'?}}
I love this testcase =)
^_^
Post by Richard Smith
Post by Kaelyn Takata
This includes adding the new TypoExpr-based lazy typo correction to
LookupMemberExprInRecord as an alternative to the existing eager typo
correction.
---
lib/Sema/SemaExprMember.cpp | 149
+++++++++++++++++++++++++++++--
test/SemaCXX/arrow-operator.cpp | 5 +-
test/SemaCXX/typo-correction-delayed.cpp | 32 +++++++
test/SemaCXX/typo-correction-pt2.cpp | 2 +-
test/SemaCXX/typo-correction.cpp | 10 +--
5 files changed, 182 insertions(+), 16 deletions(-)
create mode 100644 test/SemaCXX/typo-correction-delayed.cpp
_______________________________________________
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.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140730/73c0413e/attachment.html>
Continue reading on narkive:
Loading...