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

[David Blaikie]

On Fri, Apr 13, 2012 at 11:57 AM, Kaelyn Uhrain <rikka at google.com> wrote:
> 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.

I wouldn't recommend using the number of clang test files failing as a
basis for deciding whether the diagnostic should be on by default -
there are all sorts of weird things in the tests. Running it over
clang itself or other code might give a better indication of the false
positive rate. If it'd be lots of busy work (without fixing bugs) to
cleanup Clang & LLVM to build with this warning on, that's a pretty
good indication that it doesn't meet the default-on bar. (of course if
it doesn't require much/any cleanup that doesn't necessarily imply
that it should be on-by-default, but gives some hope & suggests
further experiments).

At least that's been my take on things. Certainly don't be afraid to
fix up/modify a bunch of existing tests if your warning starts firing
on them. In some cases it's easier/the right thing to just turn the
warning off in those tests, or simply to add the // expected-warning
annotations in if it's just a few, for example.

- David

> 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
>>> >
>>
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>