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