<div dir="ltr">The new patch looks pretty good except for one thing: the handling of a typo correction that removed the name qualifier.<div><br></div><div>Right now in the case where the best correction is an unqualified name, the error message suggests replacing the qualified name with the unqualified one in the "using" statement but will sometimes include a FixIt to remove the entire "using" statement. Since a "using" statement with an unqualified name is not valid syntax, you need to either change the error message to suggest dropping the "using" statement in this situation, or you need to change UsingValidatorCCC::ValidateCandidate to exclude any correction candidates that would drop the qualifier. Since removing entire statements is a big stretch beyond typo correction (which just suggests alternate, potentially qualified, identifiers), I vote strongly for the latter option of rejecting candidates that would result in having an unqualified name in the "using" statement--which is to say, reject candidates where droppedSpecifier would be true, and in the diagnostic message just use a literal 0 since droppedSpecifier would never be true.</div>
<div><br></div><div>As a minor nit, your change to Sema::BuildUsingDeclaration can be further simplified by dropping the "if" from "if (NamedDecl *ND = Corrected.getCorrectionDecl())" since UsingValidatorCCC rejects any candidate that does not have a NamedDecl. Once that "if" is gone you can drop the second "if (R.empty())" and just mark the UsingDecl as invalid, etc, if typo correction fails.</div>
<div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 9, 2013 at 10:46 AM, Luke Zarko <span dir="ltr"><<a href="mailto:zarko@google.com" target="_blank" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=zarko@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">zarko@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 class="im">Thanks for the feedback!<br>
<br>
1- I've removed the guard.<br>
2- Done. I've also added a FixIt to remove using decls that typo<br>
correction claims have unnecessary specifiers.<br>
3- Done; I believe I understand what droppedSpecifier means, but<br>
there's a few tests I added in test/SemaCXX/using-decl-1.cpp that seem<br>
to show that it's maybe not making all the corrections it might be<br>
able to (is droppedSpecifier all-or-nothing?)<br>
<br>
On Mon, Jul 8, 2013 at 11:14 AM, Kaelyn Uhrain <<a href="mailto:rikka@google.com" 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>> wrote:<br>
</div><div class=""><div class="h5">> Luke,<br>
><br>
> I just looked through the patch and have a few questions/comments about it:<br>
><br>
> 1) Why do you only perform typo correction when the scope is not NULL? I did<br>
> not see any issues within tools/clang/test/ with the guard removed. Plus, as<br>
> I mentioned on IRC, there is only one path in typo correction that<br>
> dereferences the scope without checking it is not NULL, that path seems to<br>
> be a hard-to-reach path and/or irrelevant in the context you are running it<br>
> (iirc, it was only reached from a very ObjC-specific code path), and is most<br>
> likely a bug in the code.<br>
><br>
> 2) Is there a reason you aren't providing a fixit hint for the typo<br>
> correction? If so, why can't one be provided? (And in the case that a fixit<br>
> cannot be provided, you need to drop the declaration of CorrectedStr since<br>
> it isn't being used.) If a fixit hint can be provided, please do and add a<br>
> test for the fixit hint to an appropriate file under test/FixIt/; basically<br>
> you just need to put one of your tests that offers a correction under there<br>
> to make sure that the fixit hint provides the right substitution in the<br>
> right place and doesn't introduce additional compilation errors.<br>
><br>
> 3) You'll need to update your patch against clang HEAD and make sure your<br>
> tests still run, since last week I submitted some typo correction changes<br>
> and diag::err_no_member_suggest takes an additional argument to support<br>
> dropping namespace qualifiers (look for declarations of "bool<br>
> droppedSpecifier", e.g. in TryNamespaceTypoCorrection in SemaDeclCXX.cpp or<br>
> in Sema::BuildCXXNestedNameSpecifier in SemaCXXScopeSpec.cpp, for examples<br>
> of how to determine if the correction is dropping the name specifier but not<br>
> changing the base identifier).<br>
><br>
> Cheers,<br>
> Kaelyn<br>
><br>
><br>
> On Mon, Jul 8, 2013 at 10:28 AM, Luke Zarko <<a href="mailto:zarko@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=zarko@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">zarko@google.com</a>> wrote:<br>
>><br>
>> Ping--is there still something that needs to be done to this?<br>
>><br>
>><br>
>> On Wed, Jun 26, 2013 at 3:04 PM, Luke Zarko <<a href="mailto:zarko@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=zarko@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">zarko@google.com</a>> wrote:<br>
>> > Thanks for looking it over!<br>
>> ><br>
>> > I've attached an updated patch with the indent fix (clang-format<br>
>> > wanted the extra two spaces for some reason) and tests for<br>
>> > IsTypeName/isa<TypeDecl> combinations. I moved the tests to<br>
>> > using-decl-1.cpp; they seemed more appropriate there.<br>
>> ><br>
>> > It's unclear how to get into the IsInstantiation case, since the<br>
>> > callers of BuildUsingDeclaration always seem to pass in 0 for Scope<br>
>> > when they pass true for instantiation<br>
>> > (TemplateDeclInstantiator::VisitUnresolvedUsingTypenameDecl,<br>
>> > TemplateDeclInstantiator::VisitUnresolvedUsingValueDecl). Using<br>
>> > getScopeForContext(CurContext) to try and get at a Scope when I need<br>
>> > it for CorrectTypo returned null. UsingValidatorCCC's checks should be<br>
>> > logically equivalent to the checks subsequent in the source file.<br>
>> ><br>
>> > On Wed, Jun 26, 2013 at 12:41 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=richard@metafoo.co.uk&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">richard@metafoo.co.uk</a>><br>
>> > wrote:<br>
>> >> On Wed, Jun 26, 2013 at 12:35 PM, Luke Zarko <<a href="mailto:zarko@google.com" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=zarko@google.com&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">zarko@google.com</a>> wrote:<br>
>> >>> I found a case where Clang doesn't yet help with typo correction:<br>
>> >>><br>
>> >>> tests/base_using.cc:<br>
>> >>><br>
>> >>> namespace Q { class AAA {}; }<br>
>> >>> using Q::AAB;<br>
>> >>><br>
>> >>> Before:<br>
>> >>><br>
>> >>> tests/base_using.cc:2:10: error: no member named 'AAB' in namespace<br>
>> >>> 'Q'<br>
>> >>> using Q::AAB;<br>
>> >>> ~~~^<br>
>> >>><br>
>> >>> After:<br>
>> >>><br>
>> >>> tests/base_using.cc:2:10: error: no member named 'AAB' in namespace<br>
>> >>> 'Q'; did you mean 'AAA'?<br>
>> >>> using Q::AAB;<br>
>> >>> ~~~^<br>
>> >>> tests/base_using.cc:1:21: note: 'AAA' declared here<br>
>> >>> namespace Q { class AAA {}; }<br>
>> >>> ^<br>
>> >><br>
>> >> Code change looks good, but could do with more test coverage for the<br>
>> >> type name / non-type-name / in instantiation cases.<br>
>> >><br>
>> >> + Diag(R.getNameLoc(), diag::err_no_member_suggest)<br>
>> >> + << NameInfo.getName() << LookupContext <<<br>
>> >> CorrectedQuotedStr<br>
>> >> + << SS.getRange();<br>
>> >><br>
>> >> We typically indent << only two spaces, not four (clang-format bug?).<br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@cs.uiuc.edu" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=cfe-commits@cs.uiuc.edu&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
</div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" onclick="window.open('https://mail.google.com/mail/?view=cm&tf=1&to=cfe-commits@cs.uiuc.edu&cc=&bcc=&su=&body=','_blank');return false;" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div></div>