nested BOOST_FOREACH and -Wshadow
Hey, I really enjoy using BOOST_FOREACH on containers, saves a ton of typing. However, one semi-annoying problem that I've ran into is that nesting instances of BOOST_FOREACH causes the gcc argument -Wshadow to complain quite a bit. A useless example of this behavior is as follows: #include <vector> #include <iostream> using namespace std; #include <boost/foreach.hpp> int main() { vector < vector <int> > array; BOOST_FOREACH( vector<int>& i1, array) BOOST_FOREACH( int i2, i1) cout << i2; } And you get the following warnings: g++ -Wshadow shadowtest.cpp -o shadowtest shadowtest.cpp: In function ‘int main()’: shadowtest.cpp:13: warning: declaration of ‘_foreach_col’ shadows a previous local shadowtest.cpp:12: warning: shadowed declaration is here shadowtest.cpp:13: warning: declaration of ‘_foreach_cur’ shadows a previous local shadowtest.cpp:12: warning: shadowed declaration is here shadowtest.cpp:13: warning: declaration of ‘_foreach_end’ shadows a previous local shadowtest.cpp:12: warning: shadowed declaration is here shadowtest.cpp:13: warning: declaration of ‘_foreach_continue’ shadows a previous local shadowtest.cpp:12: warning: shadowed declaration is here Now, I know that some people don't believe in getting rid of warnings just for warnings sake, but the fix for this would be extremely trivial and useful (given the amount of noise that it generates) -- just add a new macro, BOOST_FOREACH_NESTED, which has a third parameter denoting the level of nesting. That parameter would be token-pasted onto the end of the variable name, and get rid of the warnings. One could define up to N more macros of the form BOOST_FOREACH_NESTED_N that get rid of the third parameter, by calling that macro appropriately. This wouldn't break any existing code, and would allow those that choose to get rid of the warnings. There are a number of arguments against such a change: (a) -Wshadow is gcc specific and isn't enabled by default, (b) If you don't like the warning, don't nest BOOST_FOREACH statements, or don't use -Wshadow. However, my feeling (and this is probably just a matter of personal preference) is that given the triviality of the fix, theres no reason to not fix it. Granted, I could easily just make my own and stick it in a header file, but it seems like it would make more sense to be in boost itself. I could create a patch and post it if so desired. Thanks! Dustin -- Innovation is just a problem away -- Innovation is just a problem away
Dustin Spicuzza wrote:
Hey,
I really enjoy using BOOST_FOREACH on containers, saves a ton of typing. However, one semi-annoying problem that I've ran into is that nesting instances of BOOST_FOREACH causes the gcc argument -Wshadow to complain quite a bit. <snip> Now, I know that some people don't believe in getting rid of warnings just for warnings sake, but the fix for this would be extremely trivial and useful (given the amount of noise that it generates) -- just add a new macro, BOOST_FOREACH_NESTED, which has a third parameter denoting the level of nesting. That parameter would be token-pasted onto the end of the variable name, and get rid of the warnings. One could define up to N more macros of the form BOOST_FOREACH_NESTED_N that get rid of the third parameter, by calling that macro appropriately. This wouldn't break any existing code, and would allow those that choose to get rid of the warnings.
I'd suggest a simpler (from the user perspective) solution would be to have BOOST_FOREACH paste __LINE__ into its variable names. Then the problem would only occur if nested BOOST_FOREACHs were used on the same line. Asking users not to do that seems a lot more reasonable than asking them not to use -Wshadow or not to use nested BOOST_FOREACHs at all. John Bytheway
John Bytheway wrote:
I'd suggest a simpler (from the user perspective) solution would be to have BOOST_FOREACH paste __LINE__ into its variable names. Then the problem would only occur if nested BOOST_FOREACHs were used on the same line. Asking users not to do that seems a lot more reasonable than asking them not to use -Wshadow or not to use nested BOOST_FOREACHs at all.
I'm pretty sure you'd need a special variant for MS compilers, since their __LINE__ expansion is not cleanly pasteable. (You can use their counting macro instead.) Sebastian
Sebastian Redl wrote:
John Bytheway wrote:
I'd suggest a simpler (from the user perspective) solution would be to have BOOST_FOREACH paste __LINE__ into its variable names. Then the problem would only occur if nested BOOST_FOREACHs were used on the same line. Asking users not to do that seems a lot more reasonable than asking them not to use -Wshadow or not to use nested BOOST_FOREACHs at all.
I'm pretty sure you'd need a special variant for MS compilers, since their __LINE__ expansion is not cleanly pasteable. (You can use their counting macro instead.)
The line macro is a great idea, definitely far simpler than what I was suggesting. However, I'm not quite familiar with the MS counting macro... Dustin -- Innovation is just a problem away
Dustin Spicuzza wrote:
Sebastian Redl wrote:
John Bytheway wrote:
I'd suggest a simpler (from the user perspective) solution would be to have BOOST_FOREACH paste __LINE__ into its variable names. Then the problem would only occur if nested BOOST_FOREACHs were used on the same line. Asking users not to do that seems a lot more reasonable than asking them not to use -Wshadow or not to use nested BOOST_FOREACHs at all.
I'm pretty sure you'd need a special variant for MS compilers, since their __LINE__ expansion is not cleanly pasteable. (You can use their counting macro instead.)
The line macro is a great idea, definitely far simpler than what I was suggesting. However, I'm not quite familiar with the MS counting macro...
It's __COUNTER__, unfolds into an integer. It gets incremented every time it gets unfolded.
At 5:33 PM +0300 2/7/09, Andrey Semashev wrote:
It's __COUNTER__, unfolds into an integer. It gets incremented every time it gets unfolded.
FWIW, this feature was also recently added to gcc: http://gcc.gnu.org/gcc-4.3/changes.html A new predefined macro __COUNTER__ has been added. It expands to sequential integral values starting from 0. In conjunction with the ## operator, this provides a convenient means to generate unique identifiers.
on Sat Feb 07 2009, Sebastian Redl <sebastian.redl-AT-getdesigned.at> wrote: F> John Bytheway wrote:
I'd suggest a simpler (from the user perspective) solution would be to have BOOST_FOREACH paste __LINE__ into its variable names. Then the problem would only occur if nested BOOST_FOREACHs were used on the same line. Asking users not to do that seems a lot more reasonable than asking them not to use -Wshadow or not to use nested BOOST_FOREACHs at all.
I'm pretty sure you'd need a special variant for MS compilers, since their __LINE__ expansion is not cleanly pasteable. (You can use their counting macro instead.)
Doesn't using BOOST_PP_CAT work? -- Dave Abrahams BoostPro Computing http://www.boostpro.com
David Abrahams wrote:
on Sat Feb 07 2009, Sebastian Redl <sebastian.redl-AT-getdesigned.at> wrote:
F> John Bytheway wrote:
I'd suggest a simpler (from the user perspective) solution would be to have BOOST_FOREACH paste __LINE__ into its variable names. Then the problem would only occur if nested BOOST_FOREACHs were used on the same line. Asking users not to do that seems a lot more reasonable than asking them not to use -Wshadow or not to use nested BOOST_FOREACHs at all.
I'm pretty sure you'd need a special variant for MS compilers, since their __LINE__ expansion is not cleanly pasteable. (You can use their counting macro instead.)
Doesn't using BOOST_PP_CAT work?
Hmm, just tried and it works on VC9. It surely didn't on VC7.1.
David Abrahams wrote:
on Sat Feb 07 2009, Sebastian Redl <sebastian.redl-AT-getdesigned.at> wrote:
F> John Bytheway wrote:
I'd suggest a simpler (from the user perspective) solution would be to have BOOST_FOREACH paste __LINE__ into its variable names. Then the problem would only occur if nested BOOST_FOREACHs were used on the same line. Asking users not to do that seems a lot more reasonable than asking them not to use -Wshadow or not to use nested BOOST_FOREACHs at all.
I'm pretty sure you'd need a special variant for MS compilers, since their __LINE__ expansion is not cleanly pasteable. (You can use their counting macro instead.)
Doesn't using BOOST_PP_CAT work?
Ok, so attached is a simple patch to BOOST_FOREACH (v1.37) using the __LINE__ mechanism suggested. It works for gcc 4.3, but I don't have access to other compilers at the moment -- and even if I did, I'm not quite sure of the best way to mix the __LINE__ and __COUNTER__ implementations correctly. Dustin -- Innovation is just a problem away --- foreach-original.hpp 2009-01-23 13:19:15.000000000 -0500 +++ foreach.hpp 2009-02-09 15:18:22.000000000 -0500 @@ -976,58 +976,58 @@ #define BOOST_FOREACH_BEGIN(COL) \ boost::foreach_detail_::begin( \ - _foreach_col \ + BOOST_PP_CAT(_foreach_col, __LINE__) \ , BOOST_FOREACH_TYPEOF(COL) \ , BOOST_FOREACH_SHOULD_COPY(COL)) #define BOOST_FOREACH_END(COL) \ boost::foreach_detail_::end( \ - _foreach_col \ + BOOST_PP_CAT(_foreach_col, __LINE__) \ , BOOST_FOREACH_TYPEOF(COL) \ , BOOST_FOREACH_SHOULD_COPY(COL)) #define BOOST_FOREACH_DONE(COL) \ boost::foreach_detail_::done( \ - _foreach_cur \ - , _foreach_end \ + BOOST_PP_CAT(_foreach_cur, __LINE__) \ + , BOOST_PP_CAT(_foreach_end, __LINE__) \ , BOOST_FOREACH_TYPEOF(COL)) #define BOOST_FOREACH_NEXT(COL) \ boost::foreach_detail_::next( \ - _foreach_cur \ + BOOST_PP_CAT(_foreach_cur, __LINE__) \ , BOOST_FOREACH_TYPEOF(COL)) #define BOOST_FOREACH_DEREF(COL) \ boost::foreach_detail_::deref( \ - _foreach_cur \ + BOOST_PP_CAT(_foreach_cur, __LINE__) \ , BOOST_FOREACH_TYPEOF(COL)) #define BOOST_FOREACH_RBEGIN(COL) \ boost::foreach_detail_::rbegin( \ - _foreach_col \ + BOOST_PP_CAT(_foreach_col, __LINE__) \ , BOOST_FOREACH_TYPEOF(COL) \ , BOOST_FOREACH_SHOULD_COPY(COL)) #define BOOST_FOREACH_REND(COL) \ boost::foreach_detail_::rend( \ - _foreach_col \ + BOOST_PP_CAT(_foreach_col, __LINE__) \ , BOOST_FOREACH_TYPEOF(COL) \ , BOOST_FOREACH_SHOULD_COPY(COL)) #define BOOST_FOREACH_RDONE(COL) \ boost::foreach_detail_::rdone( \ - _foreach_cur \ - , _foreach_end \ + BOOST_PP_CAT(_foreach_cur, __LINE__) \ + , BOOST_PP_CAT(_foreach_end, __LINE__) \ , BOOST_FOREACH_TYPEOF(COL)) #define BOOST_FOREACH_RNEXT(COL) \ boost::foreach_detail_::rnext( \ - _foreach_cur \ + BOOST_PP_CAT(_foreach_cur, __LINE__) \ , BOOST_FOREACH_TYPEOF(COL)) #define BOOST_FOREACH_RDEREF(COL) \ boost::foreach_detail_::rderef( \ - _foreach_cur \ + BOOST_PP_CAT(_foreach_cur, __LINE__) \ , BOOST_FOREACH_TYPEOF(COL)) /////////////////////////////////////////////////////////////////////////////// @@ -1058,14 +1058,20 @@ // #define BOOST_FOREACH(VAR, COL) \ BOOST_FOREACH_PREAMBLE() \ - if (boost::foreach_detail_::auto_any_t _foreach_col = BOOST_FOREACH_CONTAIN(COL)) {} else \ - if (boost::foreach_detail_::auto_any_t _foreach_cur = BOOST_FOREACH_BEGIN(COL)) {} else \ - if (boost::foreach_detail_::auto_any_t _foreach_end = BOOST_FOREACH_END(COL)) {} else \ - for (bool _foreach_continue = true; \ - _foreach_continue && !BOOST_FOREACH_DONE(COL); \ - _foreach_continue ? BOOST_FOREACH_NEXT(COL) : (void)0) \ - if (boost::foreach_detail_::set_false(_foreach_continue)) {} else \ - for (VAR = BOOST_FOREACH_DEREF(COL); !_foreach_continue; _foreach_continue = true) + if (boost::foreach_detail_::auto_any_t \ + BOOST_PP_CAT(_foreach_col , __LINE__ ) = BOOST_FOREACH_CONTAIN(COL)) {} else \ + if (boost::foreach_detail_::auto_any_t \ + BOOST_PP_CAT(_foreach_cur , __LINE__ ) = BOOST_FOREACH_BEGIN(COL)) {} else \ + if (boost::foreach_detail_::auto_any_t \ + BOOST_PP_CAT(_foreach_end , __LINE__ ) = BOOST_FOREACH_END(COL)) {} else \ + for (bool BOOST_PP_CAT(_foreach_continue , __LINE__ ) = true; \ + BOOST_PP_CAT(_foreach_continue , __LINE__ ) && !BOOST_FOREACH_DONE(COL); \ + BOOST_PP_CAT(_foreach_continue , __LINE__ ) ? BOOST_FOREACH_NEXT(COL) : (void)0) \ + if (boost::foreach_detail_::set_false(BOOST_PP_CAT(_foreach_continue , __LINE__ ))) {} \ + else \ + for (VAR = BOOST_FOREACH_DEREF(COL); \ + !BOOST_PP_CAT(_foreach_continue , __LINE__ ); \ + BOOST_PP_CAT(_foreach_continue , __LINE__ ) = true) /////////////////////////////////////////////////////////////////////////////// // BOOST_REVERSE_FOREACH @@ -1076,13 +1082,19 @@ // #define BOOST_REVERSE_FOREACH(VAR, COL) \ BOOST_FOREACH_PREAMBLE() \ - if (boost::foreach_detail_::auto_any_t _foreach_col = BOOST_FOREACH_CONTAIN(COL)) {} else \ - if (boost::foreach_detail_::auto_any_t _foreach_cur = BOOST_FOREACH_RBEGIN(COL)) {} else \ - if (boost::foreach_detail_::auto_any_t _foreach_end = BOOST_FOREACH_REND(COL)) {} else \ - for (bool _foreach_continue = true; \ - _foreach_continue && !BOOST_FOREACH_RDONE(COL); \ - _foreach_continue ? BOOST_FOREACH_RNEXT(COL) : (void)0) \ - if (boost::foreach_detail_::set_false(_foreach_continue)) {} else \ - for (VAR = BOOST_FOREACH_RDEREF(COL); !_foreach_continue; _foreach_continue = true) + if (boost::foreach_detail_::auto_any_t \ + BOOST_PP_CAT(_foreach_col , __LINE__ ) = BOOST_FOREACH_CONTAIN(COL)) {} else \ + if (boost::foreach_detail_::auto_any_t \ + BOOST_PP_CAT(_foreach_cur , __LINE__ ) = BOOST_FOREACH_RBEGIN(COL)) {} else \ + if (boost::foreach_detail_::auto_any_t \ + BOOST_PP_CAT(_foreach_end , __LINE__ ) = BOOST_FOREACH_REND(COL)) {} else \ + for (bool BOOST_PP_CAT(_foreach_continue , __LINE__ ) = true; \ + BOOST_PP_CAT(_foreach_continue , __LINE__ ) && !BOOST_FOREACH_RDONE(COL); \ + BOOST_PP_CAT(_foreach_continue , __LINE__ ) ? BOOST_FOREACH_RNEXT(COL) : (void)0) \ + if (boost::foreach_detail_::set_false(BOOST_PP_CAT(_foreach_continue , __LINE__ ))) {} \ + else \ + for (VAR = BOOST_FOREACH_RDEREF(COL); \ + !BOOST_PP_CAT(_foreach_continue , __LINE__ ); \ + BOOST_PP_CAT(_foreach_continue , __LINE__ ) = true) #endif
Dustin Spicuzza wrote:
David Abrahams wrote:
on Sat Feb 07 2009, Sebastian Redl <sebastian.redl-AT-getdesigned.at> wrote:
John Bytheway wrote:
I'd suggest a simpler (from the user perspective) solution would be to have BOOST_FOREACH paste __LINE__ into its variable names. Then the problem would only occur if nested BOOST_FOREACHs were used on the same line. Asking users not to do that seems a lot more reasonable than asking them not to use -Wshadow or not to use nested BOOST_FOREACHs at all.
I'm pretty sure you'd need a special variant for MS compilers, since their __LINE__ expansion is not cleanly pasteable. (You can use their counting macro instead.)
Doesn't using BOOST_PP_CAT work?
Ok, so attached is a simple patch to BOOST_FOREACH (v1.37) using the __LINE__ mechanism suggested. It works for gcc 4.3, but I don't have access to other compilers at the moment -- and even if I did, I'm not quite sure of the best way to mix the __LINE__ and __COUNTER__ implementations correctly.
Thanks for the patch. I've made my own minor modifications and committed it to trunk. Since I also recall there being problems on msvc with __LINE__ and msvc's edit-and-continue feature, I'm simply not mangling names for msvc. That's ok because msvc doesn't emit shadow warnings, AFAIK. (Using __COUNTER__ on msvc would actually be wrong because it would become impossible to refer back to an identifier created previously.) -- Eric Niebler BoostPro Computing http://www.boostpro.com
Dustin Spicuzza wrote:
shadowtest.cpp:13: warning: declaration of ‘_foreach_col’ shadows a previous local shadowtest.cpp:12: warning: shadowed declaration is here shadowtest.cpp:13: warning: declaration of ‘_foreach_cur’ shadows a previous local shadowtest.cpp:12: warning: shadowed declaration is here shadowtest.cpp:13: warning: declaration of ‘_foreach_end’ shadows a previous local shadowtest.cpp:12: warning: shadowed declaration is here shadowtest.cpp:13: warning: declaration of ‘_foreach_continue’ shadows a previous local shadowtest.cpp:12: warning: shadowed declaration is here
You get similar warnings to this with VC9 if you build with code analysis enabled. I've silenced this in my local copy by extending the existing BOOST_FOREACH_SUPPRESS_WARNINGS macro to suppress warning C6246 as well as the existing C6001. -- View this message in context: http://www.nabble.com/nested-BOOST_FOREACH-and--Wshadow-tp21886255p21889459.... Sent from the Boost - Dev mailing list archive at Nabble.com.
participants (8)
-
Andrey Semashev -
David Abrahams -
Dustin Spicuzza -
Eric Niebler -
John Bytheway -
Kim Barrett -
Richard Webb -
Sebastian Redl