The latest patch looks good to me. Thanks for expanding the coverage of Clang's typo correction! :)
On Tue, Jul 9, 2013 at 4:46 PM, Luke Zarko <[email protected]> wrote: > Awesome. I've updated the patch with the change you suggested. Thanks > again for your help! > > On Tue, Jul 9, 2013 at 3:37 PM, Kaelyn Uhrain <[email protected]> wrote: > > That looks much better! > > > > One last note: in UsingValidatorCCC, instead of defining > droppedSpecifier as > > "Candidate.WillReplaceSpecifier() && LookupName == > > Candidate.getAsString(LangOpts)", it would be faster and safer to define > it > > as "Candidate.WillReplaceSpecifier() && > > !Candidate.getCorrectionSpecifier()". The former is checking specifically > > for changing "a::foo" to "foo" (dropping the qualifier from the current > > name), while the latter would also match changing "a::foo" to "bar" The > > latter is safer since a completely unqualified name is invalid for a > "using" > > declaration, and faster since it doesn't have to build a string > > representation of the possibly-qualified name and then do a string > > comparison, but simply checks that the candidate has a > NestedNameSpecifier* > > if it is going to replace the current specifier. And UsingValidation > won't > > need LookupName or LangOpts. :) > > > > > > On Tue, Jul 9, 2013 at 3:23 PM, Luke Zarko <[email protected]> wrote: > >> > >> That cleaned things up a bit. Now for the cases where the code used to > >> remove the using decl it instead suggests a fully-qualified > >> replacement to preserve syntax correctness while still allowing the > >> compilation to continue: > >> > >> namespace using_suggestion_ty_dropped_specifier { > >> class AAA {}; // expected-note > >> {{'::using_suggestion_ty_dropped_specifier::AAA' declared here}} > >> namespace N { } > >> using N::AAA; // expected-error {{no member named 'AAA' in namespace > >> 'using_suggestion_ty_dropped_specifier::N'; did you mean > >> '::using_suggestion_ty_dropped_specifier::AAA'?}} > >> } > >> > >> > >> On Tue, Jul 9, 2013 at 1:09 PM, Kaelyn Uhrain <[email protected]> wrote: > >> > The new patch looks pretty good except for one thing: the handling of > a > >> > typo > >> > correction that removed the name qualifier. > >> > > >> > 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. > >> > > >> > 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. > >> > > >> > On Tue, Jul 9, 2013 at 10:46 AM, Luke Zarko <[email protected]> wrote: > >> >> > >> >> Thanks for the feedback! > >> >> > >> >> 1- I've removed the guard. > >> >> 2- Done. I've also added a FixIt to remove using decls that typo > >> >> correction claims have unnecessary specifiers. > >> >> 3- Done; I believe I understand what droppedSpecifier means, but > >> >> there's a few tests I added in test/SemaCXX/using-decl-1.cpp that > seem > >> >> to show that it's maybe not making all the corrections it might be > >> >> able to (is droppedSpecifier all-or-nothing?) > >> >> > >> >> On Mon, Jul 8, 2013 at 11:14 AM, Kaelyn Uhrain <[email protected]> > >> >> wrote: > >> >> > Luke, > >> >> > > >> >> > I just looked through the patch and have a few questions/comments > >> >> > about > >> >> > it: > >> >> > > >> >> > 1) Why do you only perform typo correction when the scope is not > >> >> > NULL? I > >> >> > did > >> >> > not see any issues within tools/clang/test/ with the guard removed. > >> >> > Plus, as > >> >> > I mentioned on IRC, there is only one path in typo correction that > >> >> > dereferences the scope without checking it is not NULL, that path > >> >> > seems > >> >> > to > >> >> > be a hard-to-reach path and/or irrelevant in the context you are > >> >> > running > >> >> > it > >> >> > (iirc, it was only reached from a very ObjC-specific code path), > and > >> >> > is > >> >> > most > >> >> > likely a bug in the code. > >> >> > > >> >> > 2) Is there a reason you aren't providing a fixit hint for the typo > >> >> > correction? If so, why can't one be provided? (And in the case > that a > >> >> > fixit > >> >> > cannot be provided, you need to drop the declaration of > CorrectedStr > >> >> > since > >> >> > it isn't being used.) If a fixit hint can be provided, please do > and > >> >> > add > >> >> > a > >> >> > test for the fixit hint to an appropriate file under test/FixIt/; > >> >> > basically > >> >> > you just need to put one of your tests that offers a correction > under > >> >> > there > >> >> > to make sure that the fixit hint provides the right substitution in > >> >> > the > >> >> > right place and doesn't introduce additional compilation errors. > >> >> > > >> >> > 3) You'll need to update your patch against clang HEAD and make > sure > >> >> > your > >> >> > tests still run, since last week I submitted some typo correction > >> >> > changes > >> >> > and diag::err_no_member_suggest takes an additional argument to > >> >> > support > >> >> > dropping namespace qualifiers (look for declarations of "bool > >> >> > droppedSpecifier", e.g. in TryNamespaceTypoCorrection in > >> >> > SemaDeclCXX.cpp > >> >> > or > >> >> > in Sema::BuildCXXNestedNameSpecifier in SemaCXXScopeSpec.cpp, for > >> >> > examples > >> >> > of how to determine if the correction is dropping the name > specifier > >> >> > but > >> >> > not > >> >> > changing the base identifier). > >> >> > > >> >> > Cheers, > >> >> > Kaelyn > >> >> > > >> >> > > >> >> > On Mon, Jul 8, 2013 at 10:28 AM, Luke Zarko <[email protected]> > wrote: > >> >> >> > >> >> >> Ping--is there still something that needs to be done to this? > >> >> >> > >> >> >> > >> >> >> On Wed, Jun 26, 2013 at 3:04 PM, Luke Zarko <[email protected]> > >> >> >> wrote: > >> >> >> > Thanks for looking it over! > >> >> >> > > >> >> >> > I've attached an updated patch with the indent fix (clang-format > >> >> >> > wanted the extra two spaces for some reason) and tests for > >> >> >> > IsTypeName/isa<TypeDecl> combinations. I moved the tests to > >> >> >> > using-decl-1.cpp; they seemed more appropriate there. > >> >> >> > > >> >> >> > It's unclear how to get into the IsInstantiation case, since the > >> >> >> > callers of BuildUsingDeclaration always seem to pass in 0 for > >> >> >> > Scope > >> >> >> > when they pass true for instantiation > >> >> >> > (TemplateDeclInstantiator::VisitUnresolvedUsingTypenameDecl, > >> >> >> > TemplateDeclInstantiator::VisitUnresolvedUsingValueDecl). Using > >> >> >> > getScopeForContext(CurContext) to try and get at a Scope when I > >> >> >> > need > >> >> >> > it for CorrectTypo returned null. UsingValidatorCCC's checks > >> >> >> > should > >> >> >> > be > >> >> >> > logically equivalent to the checks subsequent in the source > file. > >> >> >> > > >> >> >> > On Wed, Jun 26, 2013 at 12:41 PM, Richard Smith > >> >> >> > <[email protected]> > >> >> >> > wrote: > >> >> >> >> On Wed, Jun 26, 2013 at 12:35 PM, Luke Zarko <[email protected] > > > >> >> >> >> wrote: > >> >> >> >>> I found a case where Clang doesn't yet help with typo > >> >> >> >>> correction: > >> >> >> >>> > >> >> >> >>> tests/base_using.cc: > >> >> >> >>> > >> >> >> >>> namespace Q { class AAA {}; } > >> >> >> >>> using Q::AAB; > >> >> >> >>> > >> >> >> >>> Before: > >> >> >> >>> > >> >> >> >>> tests/base_using.cc:2:10: error: no member named 'AAB' in > >> >> >> >>> namespace > >> >> >> >>> 'Q' > >> >> >> >>> using Q::AAB; > >> >> >> >>> ~~~^ > >> >> >> >>> > >> >> >> >>> After: > >> >> >> >>> > >> >> >> >>> tests/base_using.cc:2:10: error: no member named 'AAB' in > >> >> >> >>> namespace > >> >> >> >>> 'Q'; did you mean 'AAA'? > >> >> >> >>> using Q::AAB; > >> >> >> >>> ~~~^ > >> >> >> >>> tests/base_using.cc:1:21: note: 'AAA' declared here > >> >> >> >>> namespace Q { class AAA {}; } > >> >> >> >>> ^ > >> >> >> >> > >> >> >> >> Code change looks good, but could do with more test coverage > for > >> >> >> >> the > >> >> >> >> type name / non-type-name / in instantiation cases. > >> >> >> >> > >> >> >> >> + Diag(R.getNameLoc(), diag::err_no_member_suggest) > >> >> >> >> + << NameInfo.getName() << LookupContext << > >> >> >> >> CorrectedQuotedStr > >> >> >> >> + << SS.getRange(); > >> >> >> >> > >> >> >> >> We typically indent << only two spaces, not four (clang-format > >> >> >> >> bug?). > >> >> >> _______________________________________________ > >> >> >> cfe-commits mailing list > >> >> >> [email protected] > >> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >> >> > > >> >> > > >> >> > >> >> _______________________________________________ > >> >> cfe-commits mailing list > >> >> [email protected] > >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >> >> > >> > > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
