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
