[PATCH v4 5/8] Start adding the infrastructure for handling TypoExprs.

[David Blaikie]

Given that these are parts of a patch series, some of which have already
been signed off on/I haven't looked at, it's a bit hard to review the
overall design - but I believe you & Richard talked that over, so I'm not
too worried about that (just explaining why my review feedback's going to
be a bit narrow)

+  if (NumTypos > 0 && !ExprEvalContexts.empty())
+    ExprEvalContexts.back().NumTypos += NumTypos;
+  else
+    assert(NumTypos == 0 && "There are outstanding typos after popping the "
+                            "last ExpressionEvaluationContextRecord");

Could this be simplified? I think it's equivalent to:

  if (!ExprEvalContexts.empty())
    ExprEvalContexts.back().NumTypos += NumTypos;
  else
    assert(NumTypos == 0 ... );

or:

  assert(ExprEvalContexts.empty() || NumTypos != 0 && ...)
  if (!ExprEvalContexts.empty())
    ...

There's an unnecessary semicolon at the end of the "while (true)" loop
in TransformTypos::Transform


I'd roll this up a bit:


+      auto &State = SemaRef.getTypoExprState(TE);
+      if (State.DiagHandler)
+        (*State.DiagHandler)(State.Consumer->getCurrentCorrection());

into:

  if (auto *Handler = SemaRef.getTypoExprState(TE).DiagHandler)
    (*DiagHandler)(State.Consumer->getCurrentCorrection());

count in the condition, plus insert in the body of the if in
TransformTypos::TransformTypoExpr would be more efficient as a single
insert-and-check (by avoiding two separate lookups into the set):
  if (TypoExprs.insert(E).second) // means this element previously
didn't exist in the set, and now does

This:

  TransformCache.find(E) != TransformCache.end()

might commonly be written as:

  TransformCache.count(E)

Optionally with "!= 0", but often count is used directly as a boolean
test, especially when it's a single set/map (not a multi set/map).


This comment didn't quite make sense to me:


+      // If corrections for the first TypoExpr have been exhausted, retry them
+      // against the next combination of substitutions for all of the other
+      // TypoExprs.

When you say "retry them" which them do you mean? The corrections for
the first TypoExpr? But what does it mean for them to be exhausted
then, if they can still be considered?

Maybe I need a bit of help with the higher level algorithm here - I
assume the idea is:

Call transform.
As it visits TypoExpr nodes, it tries their first typo candidate available.
If we get to the end and have invalid code, reset the state of all the
typo corrections except the first one (on its next query, it'll give
back its second alternative).
If we get to the end, have invalid code, and the first typo has no
alternatives left to try - I guess we just want to print errors for
everything, without typo corrections?

It might be helpful to split out some of these chunks (the state
resetting code and maybe the diagnostic printing at the end) into
separate functions to give them logical names & make the higher level
algorithm more clear?

The loop at the end ("for (auto E : TypoExprs) {") is over a hashing
data structure, right? Is that going to cause unstable output of
diagnostics? (eg: different machines, different memory layout, the
same program might get the typos printed in a different order) or is
there something else that will fix that?

If you have to change the set to be a SetVector, or similar (to
stabilize output) - then you won't need the FirstTypoExpr anymore -
it'll just be the first one in the set, since you iterate in insertion
order at that point.

"while (TypoCorrection TC = State.Consumer->getNextCorrection()) {"
doesn't seem to be a loop - it has an unconditional return at the end
of the body. Should it just be an 'if'?


On Thu, Sep 25, 2014 at 2:00 PM, Kaelyn Takata <rikka at google.com> wrote:

> ping
>
> On Tue, Aug 26, 2014 at 11:05 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:
>>
>>>
>>> Part of the infrastructure is a map from a TypoExpr to the Sema-specific
>>> state needed to correct it, along with helpers to ease dealing with the
>>> state.
>>>
>>> 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           |  44 +++++
>>>  include/clang/Sema/SemaInternal.h   |  15 +-
>>>  include/clang/Sema/TypoCorrection.h |   2 +-
>>>  lib/Sema/SemaExpr.cpp               |   7 +
>>>  lib/Sema/SemaExprCXX.cpp            | 108 ++++++++++++
>>>  lib/Sema/SemaLookup.cpp             | 316
>>> ++++++++++++++++++++++++------------
>>>  6 files changed, 384 insertions(+), 108 deletions(-)
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141003/13621d3a/attachment.html>