<div dir="ltr">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)<br><br><pre style="color:rgb(0,0,0)">+ 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 <span style="font-family:arial">TransformTypos::Transform</span><br>
I'd roll this up a bit:
<pre>+ 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::<span style="font-family:arial">TransformTypoExpr would be more efficient as a single insert-and-check (by avoiding two separate lookups into the set):
</span>
if (TypoExprs.insert(E).second) // means this element previously didn't exist in the set, and now does
This:
<pre> TransformCache.find(E) != TransformCache.end()</pre>might commonly be written as:
<pre> 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).
</pre><pre>This comment didn't quite make sense to me:
<pre>+ // 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 ("<span style="font-family:arial">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.
"</span><span style="font-family:arial">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'?</span></pre>
</pre></pre>
</pre></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 25, 2014 at 2:00 PM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">ping</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 26, 2014 at 11:05 AM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br>+dblaikie</div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 31, 2014 at 1:20 PM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Part of the infrastructure is a map from a TypoExpr to the Sema-specific<br>
state needed to correct it, along with helpers to ease dealing with the<br>
state.<br>
<br>
The the typo count is propagated up the stack of<br>
ExpressionEvaluationContextRecords when one is popped off of to<br>
avoid accidentally dropping TypoExprs on the floor. For example,<br>
the attempted correction of g() in test/CXX/class/class.mem/p5-0x.cpp<br>
happens with an ExpressionEvaluationContextRecord that is popped off<br>
the stack prior to ActOnFinishFullExpr being called and the tree<br>
transform for TypoExprs being run.<br>
---<br>
include/clang/Sema/Sema.h | 44 +++++<br>
include/clang/Sema/SemaInternal.h | 15 +-<br>
include/clang/Sema/TypoCorrection.h | 2 +-<br>
lib/Sema/SemaExpr.cpp | 7 +<br>
lib/Sema/SemaExprCXX.cpp | 108 ++++++++++++<br>
lib/Sema/SemaLookup.cpp | 316 ++++++++++++++++++++++++------------<br>
6 files changed, 384 insertions(+), 108 deletions(-)<br>
<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>