<div dir="ltr"><div style>Luke,</div><div style><br></div>I just looked through the patch and have a few questions/comments about it:<div><br></div><div>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.</div>
<div style><br></div><div style>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.</div>
<div style><br></div><div style>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).<br>
</div><div style><br></div><div style>Cheers,</div><div style>Kaelyn</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 8, 2013 at 10:28 AM, 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">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>> 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 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 need<br>
> it for CorrectTypo returned null. UsingValidatorCCC's checks should 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 <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>> On Wed, Jun 26, 2013 at 12:35 PM, Luke Zarko <<a href="mailto:zarko@google.com">zarko@google.com</a>> wrote:<br>
>>> I found a case where Clang doesn't yet help with typo 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 namespace 'Q'<br>
>>> using Q::AAB;<br>
>>> ~~~^<br>
>>><br>
>>> After:<br>
>>><br>
>>> tests/base_using.cc:2:10: error: no member named 'AAB' in 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 the<br>
>> type name / non-type-name / in instantiation cases.<br>
>><br>
>> + Diag(R.getNameLoc(), diag::err_no_member_suggest)<br>
>> + << NameInfo.getName() << LookupContext << CorrectedQuotedStr<br>
>> + << SS.getRange();<br>
>><br>
>> We typically indent << only two spaces, not four (clang-format 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>
</blockquote></div><br></div>