<div dir="ltr">The latest patch looks good to me. Thanks for expanding the coverage of Clang's typo correction! :)</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 9, 2013 at 4:46 PM, Luke Zarko <span dir="ltr"><<a href="mailto:zarko@google.com" target="_blank">zarko@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Awesome. I've updated the patch with the change you suggested. Thanks<br>
again for your help!<br>
<div class="HOEnZb"><div class="h5"><br>
On Tue, Jul 9, 2013 at 3:37 PM, Kaelyn Uhrain <<a href="mailto:rikka@google.com">rikka@google.com</a>> wrote:<br>
> That looks much better!<br>
><br>
> One last note: in UsingValidatorCCC, instead of defining droppedSpecifier as<br>
> "Candidate.WillReplaceSpecifier() && LookupName ==<br>
> Candidate.getAsString(LangOpts)", it would be faster and safer to define it<br>
> as "Candidate.WillReplaceSpecifier() &&<br>
> !Candidate.getCorrectionSpecifier()". The former is checking specifically<br>
> for changing "a::foo" to "foo" (dropping the qualifier from the current<br>
> name), while the latter would also match changing "a::foo" to "bar" The<br>
> latter is safer since a completely unqualified name is invalid for a "using"<br>
> declaration, and faster since it doesn't have to build a string<br>
> representation of the possibly-qualified name and then do a string<br>
> comparison, but simply checks that the candidate has a NestedNameSpecifier*<br>
> if it is going to replace the current specifier. And UsingValidation won't<br>
> need LookupName or LangOpts. :)<br>
><br>
><br>
> On Tue, Jul 9, 2013 at 3:23 PM, Luke Zarko <<a href="mailto:zarko@google.com">zarko@google.com</a>> wrote:<br>
>><br>
>> That cleaned things up a bit. Now for the cases where the code used to<br>
>> remove the using decl it instead suggests a fully-qualified<br>
>> replacement to preserve syntax correctness while still allowing the<br>
>> compilation to continue:<br>
>><br>
>> namespace using_suggestion_ty_dropped_specifier {<br>
>> class AAA {}; // expected-note<br>
>> {{'::using_suggestion_ty_dropped_specifier::AAA' declared here}}<br>
>> namespace N { }<br>
>> using N::AAA; // expected-error {{no member named 'AAA' in namespace<br>
>> 'using_suggestion_ty_dropped_specifier::N'; did you mean<br>
>> '::using_suggestion_ty_dropped_specifier::AAA'?}}<br>
>> }<br>
>><br>
>><br>
>> On Tue, Jul 9, 2013 at 1:09 PM, Kaelyn Uhrain <<a href="mailto:rikka@google.com">rikka@google.com</a>> wrote:<br>
>> > The new patch looks pretty good except for one thing: the handling of a<br>
>> > typo<br>
>> > correction that removed the name qualifier.<br>
>> ><br>
>> > Right now in the case where the best correction is an unqualified name,<br>
>> > the<br>
>> > error message suggests replacing the qualified name with the unqualified<br>
>> > one<br>
>> > in the "using" statement but will sometimes include a FixIt to remove<br>
>> > the<br>
>> > entire "using" statement. Since a "using" statement with an unqualified<br>
>> > name<br>
>> > is not valid syntax, you need to either change the error message to<br>
>> > suggest<br>
>> > dropping the "using" statement in this situation, or you need to change<br>
>> > UsingValidatorCCC::ValidateCandidate to exclude any correction<br>
>> > candidates<br>
>> > that would drop the qualifier. Since removing entire statements is a big<br>
>> > stretch beyond typo correction (which just suggests alternate,<br>
>> > potentially<br>
>> > qualified, identifiers), I vote strongly for the latter option of<br>
>> > rejecting<br>
>> > candidates that would result in having an unqualified name in the<br>
>> > "using"<br>
>> > statement--which is to say, reject candidates where droppedSpecifier<br>
>> > would<br>
>> > be true, and in the diagnostic message just use a literal 0 since<br>
>> > droppedSpecifier would never be true.<br>
>> ><br>
>> > As a minor nit, your change to Sema::BuildUsingDeclaration can be<br>
>> > further<br>
>> > simplified by dropping the "if" from "if (NamedDecl *ND =<br>
>> > Corrected.getCorrectionDecl())" since UsingValidatorCCC rejects any<br>
>> > candidate that does not have a NamedDecl. Once that "if" is gone you can<br>
>> > drop the second "if (R.empty())" and just mark the UsingDecl as invalid,<br>
>> > etc, if typo correction fails.<br>
>> ><br>
>> > On Tue, Jul 9, 2013 at 10:46 AM, Luke Zarko <<a href="mailto:zarko@google.com">zarko@google.com</a>> wrote:<br>
>> >><br>
>> >> 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">rikka@google.com</a>><br>
>> >> wrote:<br>
>> >> > Luke,<br>
>> >> ><br>
>> >> > I just looked through the patch and have a few questions/comments<br>
>> >> > about<br>
>> >> > it:<br>
>> >> ><br>
>> >> > 1) Why do you only perform typo correction when the scope is not<br>
>> >> > NULL? I<br>
>> >> > did<br>
>> >> > not see any issues within tools/clang/test/ with the guard removed.<br>
>> >> > 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<br>
>> >> > seems<br>
>> >> > to<br>
>> >> > be a hard-to-reach path and/or irrelevant in the context you are<br>
>> >> > running<br>
>> >> > it<br>
>> >> > (iirc, it was only reached from a very ObjC-specific code path), and<br>
>> >> > is<br>
>> >> > 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<br>
>> >> > fixit<br>
>> >> > cannot be provided, you need to drop the declaration of CorrectedStr<br>
>> >> > since<br>
>> >> > it isn't being used.) If a fixit hint can be provided, please do and<br>
>> >> > add<br>
>> >> > a<br>
>> >> > test for the fixit hint to an appropriate file under test/FixIt/;<br>
>> >> > basically<br>
>> >> > you just need to put one of your tests that offers a correction under<br>
>> >> > there<br>
>> >> > to make sure that the fixit hint provides the right substitution in<br>
>> >> > 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<br>
>> >> > your<br>
>> >> > tests still run, since last week I submitted some typo correction<br>
>> >> > changes<br>
>> >> > and diag::err_no_member_suggest takes an additional argument to<br>
>> >> > support<br>
>> >> > dropping namespace qualifiers (look for declarations of "bool<br>
>> >> > droppedSpecifier", e.g. in TryNamespaceTypoCorrection in<br>
>> >> > SemaDeclCXX.cpp<br>
>> >> > or<br>
>> >> > in Sema::BuildCXXNestedNameSpecifier in SemaCXXScopeSpec.cpp, for<br>
>> >> > examples<br>
>> >> > of how to determine if the correction is dropping the name specifier<br>
>> >> > but<br>
>> >> > 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">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">zarko@google.com</a>><br>
>> >> >> 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<br>
>> >> >> > 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<br>
>> >> >> > need<br>
>> >> >> > it for CorrectTypo returned null. UsingValidatorCCC's checks<br>
>> >> >> > should<br>
>> >> >> > 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<br>
>> >> >> > <<a href="mailto:richard@metafoo.co.uk">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">zarko@google.com</a>><br>
>> >> >> >> wrote:<br>
>> >> >> >>> I found a case where Clang doesn't yet help with typo<br>
>> >> >> >>> 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<br>
>> >> >> >>> 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<br>
>> >> >> >>> 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<br>
>> >> >> >> 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<br>
>> >> >> >> bug?).<br>
>> >> >> _______________________________________________<br>
>> >> >> cfe-commits mailing list<br>
>> >> >> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> >> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >> ><br>
>> >> ><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> cfe-commits mailing list<br>
>> >> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>