I just realized that this check probably only applies to C++ code since (unless things changed in a recent standard) C doesn't allow struct/enum names to be used without the corresponding keyword, the way C++ does. I've updated the patch to only apply the check for C++, which reduced the number of unit tests that fail with the flag default-enabled to 13.<div>
<br></div><div>Cheers,</div><div>Kaelyn<br><br><div class="gmail_quote">On Fri, Apr 13, 2012 at 11:29 AM, Kaelyn Uhrain <span dir="ltr"><<a href="mailto:rikka@google.com">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks for the review Manuel!<br><br><div class="gmail_quote"><div class="im">On Fri, Apr 13, 2012 at 9:13 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">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">
+ "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></blockquote><div><br></div></div><div>Ah, thanks for catching that! I was too focused on the "enum" case when figuring out the wording. ;)</div><div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+void Sema::ActOnStartFunctionDeclarator(Scope* S, Declarator& D) {<br>
<br>
* and & to the variable.<br></blockquote><div><br></div></div><div>Fixed. Though I just noticed the same issue is present for ActOnFunctionDeclarator (where I'd copied the two parameters from) and three other ActOn*Decl* functions a little above ActOnStartFunctionDeclarator in Sema.h.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+ 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></blockquote><div><br></div></div><div>Good point; changed to dyn_cast.</div><div><br></div><div>I've attached a new version that also limits which shadowings it warns on to tag types found in the current lexical context, reducing the number of other unit test failures from 27 to 20. I also switched the warning to DefaultIgnore to take care of the remaining 20 failures (since the tests otherwise work as expected).</div>
<div><div></div><div class="h5">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
/Manuel<br>
<div><div></div><div><br>
On Thu, Apr 12, 2012 at 10:05 PM, Kaelyn Uhrain <<a href="mailto:rikka@google.com" target="_blank">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" target="_blank">rikka@google.com</a>> 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 errors<br>
>> without any real indication of what went wrong (the first error is the only<br>
>> remotely helpful one, but it doesn't give any reason why an 'enum' tag 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 '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 that<br>
>> modifying Sema::ActOnStartFunctionDeclarator is the right place for 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 errors<br>
>> seem to involve a lot of interactions between the parser and sema and 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>
</div></div>> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">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>
</blockquote></div></div></div><br>
</blockquote></div><br></div>