[cfe-commits] [PATCH] Add a warning for a function declaration that shadows a tag type

[Kaelyn Uhrain]

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.

Cheers,
Kaelyn

On Fri, Apr 13, 2012 at 11:29 AM, Kaelyn Uhrain <rikka at google.com> wrote:

> Thanks for the review Manuel!
>
> On Fri, Apr 13, 2012 at 9:13 AM, Manuel Klimek <klimek at google.com> wrote:
>
>> +  "function %0 shadows %1 %0; uses of %1 %0 will require an '%1' tag">,
>>
>> I'd use 'a' or a selector for enum.
>>
>
> Ah, thanks for catching that! I was too focused on the "enum" case when
> figuring out the wording. ;)
>
>
>>
>> +void Sema::ActOnStartFunctionDeclarator(Scope* S, Declarator& D) {
>>
>> * and & to the variable.
>>
>
> 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.
>
>
>> +      if (TagDecl *TD = dyn_cast_or_null<TagDecl>(*I)) {
>>
>> I've searched, but couldn't find why *I would ever be NULL here...
>>
>
> Good point; changed to dyn_cast.
>
> 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).
>
>
>> Cheers,
>> /Manuel
>>
>> On Thu, Apr 12, 2012 at 10:05 PM, Kaelyn Uhrain <rikka at google.com> wrote:
>> > Ping.
>> >
>> > Unless I hear something, I'll go ahead and commit the patch tomorrow
>> > morning.
>> >
>> >
>> > On Wed, Apr 11, 2012 at 1:44 PM, Kaelyn Uhrain <rikka at google.com>
>> wrote:
>> >>
>> >> Hi,
>> >>
>> >> This warning is to help make code like:
>> >>
>> >> class Foo {
>> >>  public:
>> >>   enum Bar { X, Y };
>> >>   void SetBar(Bar bar);  // Setter
>> >>   Bar Bar()  // Getter
>> >>  private:
>> >>   Bar bar_;
>> >> };
>> >> void Foo::SetBar(Bar bar) { bar_ = bar; }
>> >> Foo::Bar Foo::Bar() { return bar_; }
>> >>
>> >>
>> >> be a bit easier to diagnose. Currently clang spits out a bunch of
>> errors
>> >> without any real indication of what went wrong (the first error is the
>> only
>> >> remotely helpful one, but it doesn't give any reason why an 'enum' tag
>> has
>> >> to be added):
>> >>
>> >> tmp.cc:7:3: error: must use 'enum' tag to refer to type 'Bar' in this
>> >> scope
>> >>   Bar bar_;
>> >>   ^
>> >>   enum
>> >> tmp.cc:9:11: error: variable has incomplete type 'void'
>> >> void Foo::SetBar(Bar bar) { bar_ = bar; }
>> >>           ^
>> >> tmp.cc:9:18: error: use of undeclared identifier 'Bar'
>> >> void Foo::SetBar(Bar bar) { bar_ = bar; }
>> >>                  ^
>> >> tmp.cc:9:26: error: expected ';' after top level declarator
>> >> void Foo::SetBar(Bar bar) { bar_ = bar; }
>> >>                          ^
>> >>                          ;
>> >> 4 errors generated.
>> >>
>> >>
>> >> With the patch, clang emits a warning before that cascade of errors:
>> >>
>> >> tmp.cc:5:7: warning: function 'Bar' shadows enum 'Bar'; uses of enum
>> 'Bar'
>> >> will require an 'enum'
>> >>       tag [-Wshadow]
>> >>   Bar Bar();  // Getter
>> >>       ^
>> >> tmp.cc:3:3: note: 'Bar' declared here
>> >>   enum Bar { X, Y };
>> >>   ^
>> >> tmp.cc:7:3: error: must use 'enum' tag to refer to type 'Bar' in this
>> >> scope
>> >> <and the rest of the messages from the first output example>
>> >>
>> >>
>> >> I'm sending the patch for pre-commit review both because I'm uncertain
>> >> about the wording of the wording, and primarily because I'm not sure
>> that
>> >> modifying Sema::ActOnStartFunctionDeclarator is the right place for the
>> >> check (though it was the best candidate I could find).
>> >>
>> >> I'd also like to clean up the errors (in a separate patch) but the
>> errors
>> >> seem to involve a lot of interactions between the parser and sema and
>> may
>> >> take a while for me to figure out how to get things to work right. ;)
>> >>
>> >> Cheers,
>> >> Kaelyn
>> >
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120413/be7a40e5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: warn-func-shadows-tag.diff
Type: application/octet-stream
Size: 3639 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120413/be7a40e5/attachment.obj>