<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 3, 2014 at 11:45 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=dblaikie@gmail.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><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())
...</pre></div></blockquote><div>Yes, the "NumTypos > 0" was unnecessary and has been removed. (FYI, the second form you suggested doesn't quite work because the assert would fail on the common case where ExprEvalContexts is not empty and there are no typos.)<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)">There's an unnecessary semicolon at the end of the "while (true)" loop in <span style="font-family:arial">TransformTypos::Transform</span></pre></div></blockquote><div>Removed. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"><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());</pre></pre></div></blockquote><div>That rollup doesn't work because State would be unknown when accessing State.Consumer in the argument to the DiagHandler. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"><pre>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</pre></pre></div></blockquote><div>I've changed the count-then-insert to just an insert since SmallPtrSetImpl::insert returns true when the item wasn't already in the set.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"><pre>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></pre></div></blockquote><div>Changed. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"><pre><pre></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).</pre></pre></pre></pre></div></blockquote><div>Here is where the confusion is at. As the transform visits each TypoExpr node it tries the first candidate available. If the first candidates of all the TypoExprs don't work, it then tries the next (and subsequent) candidates of only the first TypoExpr that was seen. When it exhausts the candidates from the first TypoExpr, it then resets the stream of candidates for the first TypoExpr and fetches the next candidate of the TypoExpr, continuing in this way until all combinations of candidates of all TypoExprs have been tried. So for 3 TypoExprs... let's say 1 has the candidates (A, B, C), 2 has the candidates (D, E, F, G) and 3 has the candidates (H, I) the iterations would test the combinations in order:</div><div><br></div><div>A D H</div><div>B D H</div><div>C D H</div><div>A E H</div><div>B E H</div><div>C E H</div><div>A F H</div><div>B F H</div><div>C F H</div><div>A G H</div><div>B G H</div><div>C G H</div><div><div>A D I</div><div>B D I</div><div>C D I</div><div>A E I</div><div>B E I</div><div>C E I</div><div>A F I</div><div>B F I</div><div>C F I</div><div>A G I</div><div>B G I</div><div>C G I</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"><pre></pre></pre></div></blockquote></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"><pre><pre><pre>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?</pre></pre></pre></pre></div></blockquote><br class=""><div>If all of the combinations of typo corrections failed to create an acceptable Expr, then the current correction for all of TypoExprs would be an empty TypoCorrection which would cause the corresponding diagnostic handlers to print out errors without suggestions.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"><pre><pre><pre>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?</pre></pre></pre></pre></div></blockquote><div>I've split the diagnostics emission and the state handling into separate helpers, and expanded their description comments. I'm not keen on the name for the state handler but it is the best name I could come up with that was even remotely descriptive.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"><pre><pre><pre>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?</span></pre></pre></pre></pre></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"><pre><pre><pre><span style="font-family:arial">
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></pre></pre></pre></pre></div></blockquote><div>Switched it from a SmallPtrSet to a SmallSetVector like it should have been. Doing so also allowed me to simplify the state handling code by being able to assume a deterministic order, and eliminating FirstTypoExpr. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"><pre><pre><pre><span style="font-family:arial">
"</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></blockquote><div>There was actually a missing check on whether the result of Sema::BuildDeclarationNameExpr was valid.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"></pre></div><div class=""><div class="h5"><div class="gmail_extra">"<span style="color:rgb(0,0,0)">+ (FullExpr.get()->isTypeDependent() ||</span><pre style="color:rgb(0,0,0)">+ FullExpr.get()->isValueDependent() ||
+ FullExpr.get()->isInstantiationDependent())) {"
What are those checks for? I don't know what condition we would be in if we had typo corrections but it wasn't type, value, or instantiation dependent. Perhaps a comment could explain that?</pre></div></div></div></blockquote><div>The check is to avoid running the tree transform on Expr trees that are known to not have a TypoExpr in them, regardless of whether the current expression evaluation context still indicates there are uncorrected typos. I've added a comment to that effect. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><div class="gmail_extra"><pre style="color:rgb(0,0,0)">
To simplify <span style="font-family:arial">Sema::clearDelayedTypo you could add MapVector::erase(const KeyType&) (given that std::map has such an operation, it seems like a reasonable one for MapVector to have).</span></pre></div></div></div></blockquote><div>Done (simple patch attached).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><div class="gmail_extra"><span style="color:rgb(0,0,0)">Sema::getTypoExprState looks like it could be a const function (it looks like it's never meant to be called when the element isn't already in the map) - using 'lookup' instead of operator[] will be a const operation on the map (& assert if the element doesn't exist in the map).<br></span></div></div></div></blockquote><div><br></div><div>Sadly I cannot do that because the operator[] cannot be used if the method is made const, and .lookup() can't be used because it returns by-value instead of by-reference and TypoExprState is not copyable:</div><div><br></div><div><div>../tools/clang/lib/Sema/SemaLookup.cpp:4600:10: warning: returning reference to local temporary object [-Wreturn-stack-address]</div><div> return DelayedTypos.lookup(TE);</div><div><br></div><div>../include/llvm/ADT/MapVector.h:88:30: error: call to implicitly-deleted copy constructor of 'const clang::Sema::TypoExprState'<br></div></div><div><div> return Pos == Map.end()? ValueT() : Vector[Pos->second].second;</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><div class="gmail_extra"><span style="color:rgb(0,0,0)"><br>Otherwise this all looks like rather neat stuff - though I'm not as able to review in detail some of the refactoring of existing typo correction stuff. It looks plausible.</span><span style="color:black"> </span></div></div></div></blockquote><div><br class="">I've attached the updated patch. Thanks for reviewing this, David!<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><div class="gmail_extra"><span style="color:black"> </span></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><div class="gmail_extra"><span style="color:black"> </span></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><div class="gmail_extra"><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" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">ping</div><div><div><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" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=rikka@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">rikka@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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>
</div></div></blockquote></div><br></div></div>