On Fri, Apr 13, 2012 at 11:57 AM, Kaelyn Uhrain <[email protected]> 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 <[email protected]> wrote: >> >> Thanks for the review Manuel! >> >> On Fri, Apr 13, 2012 at 9:13 AM, Manuel Klimek <[email protected]> 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 <[email protected]> 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 <[email protected]> >>> > 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 >>> > [email protected] >>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> > >> >> > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
