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 >> >> >> > > >
using-typo-v5.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
