<div class="gmail_quote">On Sat, Apr 14, 2012 at 3:44 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com">klimek@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Fri, Apr 13, 2012 at 7:57 PM, Kaelyn Uhrain <<a href="mailto:rikka@google.com">rikka@google.com</a>> wrote:<br>
> I just realized that this check probably only applies to C++ code since<br>
> (unless things changed in a recent standard) C doesn't allow struct/enum<br>
> names to be used without the corresponding keyword, the way C++ does. I've<br>
> updated the patch to only apply the check for C++, which reduced the number<br>
> of unit tests that fail with the flag default-enabled to 13.<br>
<br>
</div>Am I missing the change that makes it only apply to C++?<br></blockquote><div><br></div><div>Yeah, I somehow screwed up recreating the patch without realizing it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Regarding turning it on by default, I'd suggest running it over our<br>
code base and seeing how often it fires...<br></blockquote><div><br></div><div>I tested the warning on our code base Friday afternoon, and found out it was far too noisy to really be useful. I've decided to shelve the warning and to focus fixing the errors that are triggered when a function name supersedes a tag type name (including the resulting issues from the parser e.g. thinking a function declaration is a couple of munged up variable declarations).</div>
<div><br></div><div>- Kaelyn</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
<font color="#888888">/Manuel<br>
</font><div><div></div><div class="h5"><br>
><br>
> Cheers,<br>
> Kaelyn<br>
><br>
><br>
> On Fri, Apr 13, 2012 at 11:29 AM, Kaelyn Uhrain <<a href="mailto:rikka@google.com">rikka@google.com</a>> wrote:<br>
>><br>
>> Thanks for the review Manuel!<br>
>><br>
>> On Fri, Apr 13, 2012 at 9:13 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>>><br>
>>> + "function %0 shadows %1 %0; uses of %1 %0 will require an '%1' tag">,<br>
>>><br>
>>> I'd use 'a' or a selector for enum.<br>
>><br>
>><br>
>> Ah, thanks for catching that! I was too focused on the "enum" case when<br>
>> figuring out the wording. ;)<br>
>><br>
>>><br>
>>><br>
>>> +void Sema::ActOnStartFunctionDeclarator(Scope* S, Declarator& D) {<br>
>>><br>
>>> * and & to the variable.<br>
>><br>
>><br>
>> Fixed. Though I just noticed the same issue is present for<br>
>> ActOnFunctionDeclarator (where I'd copied the two parameters from) and<br>
>> three other ActOn*Decl* functions a little<br>
>> above ActOnStartFunctionDeclarator in Sema.h.<br>
>><br>
>>><br>
>>> + if (TagDecl *TD = dyn_cast_or_null<TagDecl>(*I)) {<br>
>>><br>
>>> I've searched, but couldn't find why *I would ever be NULL here...<br>
>><br>
>><br>
>> Good point; changed to dyn_cast.<br>
>><br>
>> I've attached a new version that also limits which shadowings it warns on<br>
>> to tag types found in the current lexical context, reducing the number of<br>
>> other unit test failures from 27 to 20. I also switched the warning to<br>
>> DefaultIgnore to take care of the remaining 20 failures (since the tests<br>
>> otherwise work as expected).<br>
>><br>
>>><br>
>>> Cheers,<br>
>>> /Manuel<br>
>>><br>
>>> On Thu, Apr 12, 2012 at 10:05 PM, Kaelyn Uhrain <<a href="mailto:rikka@google.com">rikka@google.com</a>> wrote:<br>
>>> > Ping.<br>
>>> ><br>
>>> > Unless I hear something, I'll go ahead and commit the patch tomorrow<br>
>>> > morning.<br>
>>> ><br>
>>> ><br>
>>> > On Wed, Apr 11, 2012 at 1:44 PM, Kaelyn Uhrain <<a href="mailto:rikka@google.com">rikka@google.com</a>><br>
>>> > wrote:<br>
>>> >><br>
>>> >> Hi,<br>
>>> >><br>
>>> >> This warning is to help make code like:<br>
>>> >><br>
>>> >> class Foo {<br>
>>> >> public:<br>
>>> >> enum Bar { X, Y };<br>
>>> >> void SetBar(Bar bar); // Setter<br>
>>> >> Bar Bar() // Getter<br>
>>> >> private:<br>
>>> >> Bar bar_;<br>
>>> >> };<br>
>>> >> void Foo::SetBar(Bar bar) { bar_ = bar; }<br>
>>> >> Foo::Bar Foo::Bar() { return bar_; }<br>
>>> >><br>
>>> >><br>
>>> >> be a bit easier to diagnose. Currently clang spits out a bunch of<br>
>>> >> errors<br>
>>> >> without any real indication of what went wrong (the first error is the<br>
>>> >> only<br>
>>> >> remotely helpful one, but it doesn't give any reason why an 'enum' tag<br>
>>> >> has<br>
>>> >> to be added):<br>
>>> >><br>
>>> >> tmp.cc:7:3: error: must use 'enum' tag to refer to type 'Bar' in this<br>
>>> >> scope<br>
>>> >> Bar bar_;<br>
>>> >> ^<br>
>>> >> enum<br>
>>> >> tmp.cc:9:11: error: variable has incomplete type 'void'<br>
>>> >> void Foo::SetBar(Bar bar) { bar_ = bar; }<br>
>>> >> ^<br>
>>> >> tmp.cc:9:18: error: use of undeclared identifier 'Bar'<br>
>>> >> void Foo::SetBar(Bar bar) { bar_ = bar; }<br>
>>> >> ^<br>
>>> >> tmp.cc:9:26: error: expected ';' after top level declarator<br>
>>> >> void Foo::SetBar(Bar bar) { bar_ = bar; }<br>
>>> >> ^<br>
>>> >> ;<br>
>>> >> 4 errors generated.<br>
>>> >><br>
>>> >><br>
>>> >> With the patch, clang emits a warning before that cascade of errors:<br>
>>> >><br>
>>> >> tmp.cc:5:7: warning: function 'Bar' shadows enum 'Bar'; uses of enum<br>
>>> >> 'Bar'<br>
>>> >> will require an 'enum'<br>
>>> >> tag [-Wshadow]<br>
>>> >> Bar Bar(); // Getter<br>
>>> >> ^<br>
>>> >> tmp.cc:3:3: note: 'Bar' declared here<br>
>>> >> enum Bar { X, Y };<br>
>>> >> ^<br>
>>> >> tmp.cc:7:3: error: must use 'enum' tag to refer to type 'Bar' in this<br>
>>> >> scope<br>
>>> >> <and the rest of the messages from the first output example><br>
>>> >><br>
>>> >><br>
>>> >> I'm sending the patch for pre-commit review both because I'm uncertain<br>
>>> >> about the wording of the wording, and primarily because I'm not sure<br>
>>> >> that<br>
>>> >> modifying Sema::ActOnStartFunctionDeclarator is the right place for<br>
>>> >> the<br>
>>> >> check (though it was the best candidate I could find).<br>
>>> >><br>
>>> >> I'd also like to clean up the errors (in a separate patch) but the<br>
>>> >> errors<br>
>>> >> seem to involve a lot of interactions between the parser and sema and<br>
>>> >> may<br>
>>> >> take a while for me to figure out how to get things to work right. ;)<br>
>>> >><br>
>>> >> Cheers,<br>
>>> >> Kaelyn<br>
>>> ><br>
>>> ><br>
>>> ><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>
>>> ><br>
>><br>
>><br>
><br>
</div></div></blockquote></div><br>