[boost.python] GCC >=4 "-fvisibility=hidden" patch
Boost.Python's sources only deal with GCC's "-fvisibility=hidden" option for versions [3.5 - 3.9]. It should also work properly when built with GCC 4 and higher. The attached patches to boost/python/detail/config.hpp and boost/python/module_init.hpp enable it to do so. Tested with GCC 4.0 (XCode 2.2) on OSX 10.4.3 (Tiger). -- Jason Kankiewicz Advanced Publishing Technology, Inc. Index: boost/python/detail/config.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/python/detail/config.hpp,v retrieving revision 1.37 diff -u -r1.37 config.hpp --- boost/python/detail/config.hpp 29 Nov 2004 21:32:14 -0000 1.37 +++ boost/python/detail/config.hpp 29 Nov 2005 20:29:13 -0000 @@ -67,9 +67,8 @@ #endif #if defined(BOOST_PYTHON_DYNAMIC_LIB) - -# if !defined(_WIN32) && !defined(__CYGWIN__) \ - && defined(__GNUC__) && __GNUC__ >= 3 && __GNUC_MINOR__ >=5 \ +# if !defined(_WIN32) && !defined(__CYGWIN__) \ + && defined(__GNUC__) && ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4) \ && !defined(BOOST_PYTHON_GCC_SYMBOL_VISIBILITY) # define BOOST_PYTHON_USE_GCC_SYMBOL_VISIBILITY # endif Index: boost/python/module_init.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/python/module_init.hpp,v retrieving revision 1.6 diff -u -r1.6 module_init.hpp --- boost/python/module_init.hpp 20 Sep 2004 12:47:31 -0000 1.6 +++ boost/python/module_init.hpp 29 Nov 2005 20:29:13 -0000 @@ -42,7 +42,7 @@ } \ void init_module_##name() -# elif (defined(__GNUC__) && __GNUC__ >= 3 && __GNUC_MINOR__ >=5) +# elif (defined(__GNUC__) && ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4)) # define BOOST_PYTHON_MODULE_INIT(name) \ void init_module_##name(); \
This is for RC_1_33_0, sorry for not mentioning it in the subject line. For nitpickers, I should have written that Boost.Python's sources only deal with GCC's "-fvisibility=hidden" option for versions [3.5 - 4.0).
Jason Kankiewicz <jkankiewicz@advpubtech.com> writes:
This is for RC_1_33_0, sorry for not mentioning it in the subject line.
For nitpickers, I should have written that Boost.Python's sources only deal with GCC's "-fvisibility=hidden" option for versions [3.5 - 4.0).
Patched. -- Dave Abrahams Boost Consulting www.boost-consulting.com
On 11/29/05 4:27 PM, "Jason Kankiewicz" <jkankiewicz@advpubtech.com> wrote:
+# elif (defined(__GNUC__) && ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4))
Shouldn't the check for GCC 3.x be: __GNUC__ == 3 && __GNUC_MINOR__ >= 5 instead of the currently over-broad version? Otherwise version numbers like 4.6 or 5.9 would match on that phrase, which is wrong. They should match on the "__GNUC__ >= 4" phrase instead. The problem phrase should only match on 3.5 through 3.9.x. -- Daryle Walker Mac, Internet, and Video Game Junkie darylew AT hotmail DOT com
Daryle Walker <darylew@hotmail.com> writes:
On 11/29/05 4:27 PM, "Jason Kankiewicz" <jkankiewicz@advpubtech.com> wrote:
+# elif (defined(__GNUC__) && ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4))
Shouldn't the check for GCC 3.x be:
__GNUC__ == 3 && __GNUC_MINOR__ >= 5
instead of the currently over-broad version? Otherwise version numbers like 4.6 or 5.9 would match on that phrase, which is wrong. They should match on the "__GNUC__ >= 4" phrase instead.
Well, that would certainly be clearer, but it is semantically equivalent, is it not?
The problem phrase should only match on 3.5 through 3.9.x.
"problem phrase?" -- Dave Abrahams Boost Consulting www.boost-consulting.com
On 11/30/05 2:07 PM, "David Abrahams" <dave@boost-consulting.com> wrote:
Daryle Walker <darylew@hotmail.com> writes:
On 11/29/05 4:27 PM, "Jason Kankiewicz" <jkankiewicz@advpubtech.com> wrote:
+# elif (defined(__GNUC__) && ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4))
Shouldn't the check for GCC 3.x be:
__GNUC__ == 3 && __GNUC_MINOR__ >= 5
instead of the currently over-broad version? Otherwise version numbers like 4.6 or 5.9 would match on that phrase, which is wrong. They should match on the "__GNUC__ >= 4" phrase instead.
Well, that would certainly be clearer, but it is semantically equivalent, is it not?
Sort of; it happens to be equivalent for the 3.x series. The phrase implies it's applicable to any higher series (4.x, 5.x, etc.), but that's wrong because it fails numbers like 4.2. The "__GNUC__ >= 4" phrase saves you from that. The next similar error may be more insidious.
The problem phrase should only match on 3.5 through 3.9.x.
"problem phrase?"
The phrase I'm originally talked about ("__GNUC__ >= 3 && __GNUC_MINOR__
=5").
-- Daryle Walker Mac, Internet, and Video Game Junkie darylew AT hotmail DOT com
Daryle Walker <darylew@hotmail.com> writes:
On 11/30/05 2:07 PM, "David Abrahams" <dave@boost-consulting.com> wrote:
Daryle Walker <darylew@hotmail.com> writes:
On 11/29/05 4:27 PM, "Jason Kankiewicz" <jkankiewicz@advpubtech.com> wrote:
+# elif (defined(__GNUC__) && ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4))
Shouldn't the check for GCC 3.x be:
__GNUC__ == 3 && __GNUC_MINOR__ >= 5
instead of the currently over-broad version? Otherwise version numbers like 4.6 or 5.9 would match on that phrase, which is wrong. They should match on the "__GNUC__ >= 4" phrase instead.
Well, that would certainly be clearer, but it is semantically equivalent, is it not?
Sort of; it happens to be equivalent for the 3.x series. The phrase implies it's applicable to any higher series (4.x, 5.x, etc.),
Isn't it?
but that's wrong because it fails numbers like 4.2.
What fails numbers like 4.2?
The "__GNUC__ >= 4" phrase saves you from that.
So, as I was saying, it's semantically equivalent. I was trying to get an idea how urgent it was to make a change, so close to the release date.
The next similar error may be more insidious.
It's a nonissue, fortunately, since the code in question does not appear anywhere in CVS. -- Dave Abrahams Boost Consulting www.boost-consulting.com
On 12/2/05 12:55 PM, "David Abrahams" <dave@boost-consulting.com> wrote:
Daryle Walker <darylew@hotmail.com> writes:
On 11/30/05 2:07 PM, "David Abrahams" <dave@boost-consulting.com> wrote:
Daryle Walker <darylew@hotmail.com> writes:
On 11/29/05 4:27 PM, "Jason Kankiewicz" <jkankiewicz@advpubtech.com> wrote:
+# elif (defined(__GNUC__) && ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4))
Shouldn't the check for GCC 3.x be:
__GNUC__ == 3 && __GNUC_MINOR__ >= 5
instead of the currently over-broad version? Otherwise version numbers like 4.6 or 5.9 would match on that phrase, which is wrong. They should match on the "__GNUC__ >= 4" phrase instead.
Well, that would certainly be clearer, but it is semantically equivalent, is it not?
Sort of; it happens to be equivalent for the 3.x series. The phrase implies it's applicable to any higher series (4.x, 5.x, etc.),
Isn't it?
It shouldn't apply to higher series; it should only be a fix for the 3.x series. As written, it's an erroneous over-generalization.
but that's wrong because it fails numbers like 4.2.
What fails numbers like 4.2?
The "__GNUC__ >= 3 && __GNUC_MINOR__ >=5" will match numbers from the 4.x series only if the "x" is at least 5. So numbers like 4.2 will mistakenly fail. You should only use this phrase if you really mean "[3.5, 3.infinity) || [4.5, 4.infinity) || [5.5, 5.infinity) || ...". But we really mean only [3.5, 3.infinity) and we should state that directly.
The "__GNUC__ >= 4" phrase saves you from that.
So, as I was saying, it's semantically equivalent. I was trying to get an idea how urgent it was to make a change, so close to the release date.
It's not a fatal error, because the [4.0, infinity) check covers up the sloppiness of the GCC 3.x check. But we shouldn't excuse the sloppiness. And we should educate now, so someone else doesn't make a more insidious version of this error later. However, for this case, you can change it at your convenience.
The next similar error may be more insidious.
It's a nonissue, fortunately, since the code in question does not appear anywhere in CVS.
-- Daryle Walker Mac, Internet, and Video Game Junkie darylew AT hotmail DOT com
In article <BAY104-DAV1332E03766A05E111E6847BF4E0@phx.gbl>, Daryle Walker <darylew@hotmail.com> wrote:
On 12/2/05 12:55 PM, "David Abrahams" <dave@boost-consulting.com> wrote:
Daryle Walker <darylew@hotmail.com> writes:
On 11/30/05 2:07 PM, "David Abrahams" <dave@boost-consulting.com> wrote:
Daryle Walker <darylew@hotmail.com> writes:
On 11/29/05 4:27 PM, "Jason Kankiewicz" <jkankiewicz@advpubtech.com> wrote:
+# elif (defined(__GNUC__) && ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4))
Shouldn't the check for GCC 3.x be:
__GNUC__ == 3 && __GNUC_MINOR__ >= 5
instead of the currently over-broad version? Otherwise version numbers like 4.6 or 5.9 would match on that phrase, which is wrong. They should match on the "__GNUC__ >= 4" phrase instead.
Well, that would certainly be clearer, but it is semantically equivalent, is it not?
Sort of; it happens to be equivalent for the 3.x series. The phrase implies it's applicable to any higher series (4.x, 5.x, etc.),
Isn't it?
It shouldn't apply to higher series; it should only be a fix for the 3.x series. As written, it's an erroneous over-generalization.
I think that this particular nit is hardly worth the amount of discussion it's generated, but I agree with Daryle. If the code says: ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4)) then changes that only apply to gcc 4 and higher will interfere with the first clause; on the other hand, if the code says ((__GNUC__ == 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4)) then changes that only apply to gcc 4 and higher will not interfere with the first clause. To see what I am trying to say, consider the case where gcc 4.5 comes out with new syntax and needs to be *excluded* by this #if statement. If you take the proposed patch: ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4)) and naively change it to ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || (__GNUC__ >= 4 && __GNUC_MINOR__ < 5)) you would not be making the correct change. You would, of course, soon discover your error, and then you would rewrite your condition to ((__GNUC__ == 3 && __GNUC_MINOR__ >=5) || (__GNUC__ >= 4 && __GNUC_MINOR__ < 5)) which is why Daryle and I believe that the change should be made now to make subsequent modifications easier. Ben -- I changed my name: <http://periodic-kingdom.org/People/NameChange.php>
Ben Artin <macdev@artins.org> writes:
I think that this particular nit is hardly worth the amount of discussion it's generated
Exactly.
which is why Daryle and I believe that the change should be made now to make subsequent modifications easier.
Do you have any idea what code you're asking to have changed? Do you realize that the code being argued over was never checked in? The current test is (boost/python/detail/config.hpp): # if !defined(_WIN32) && !defined(__CYGWIN__) \ && !defined(BOOST_PYTHON_USE_GCC_SYMBOL_VISIBILITY) \ && BOOST_WORKAROUND(__GNUC__, >= 3) && (__GNUC_MINOR__ >=5 || __GNUC__ > 3) # define BOOST_PYTHON_USE_GCC_SYMBOL_VISIBILITY 1 # endif Do you have a problem with that? I can imagine that the following would be a very minor improvement, but it hardly seems to be worth the bits we're spending on it. # if !defined(_WIN32) && !defined(__CYGWIN__) \ && !defined(BOOST_PYTHON_USE_GCC_SYMBOL_VISIBILITY) \ && (BOOST_WORKAROUND(__GNUC__, == 3) && __GNUC_MINOR__ >=5 \ || BOOST_WORKAROUND(__GNUC__, > 3)) # define BOOST_PYTHON_USE_GCC_SYMBOL_VISIBILITY 1 # endif -- Dave Abrahams Boost Consulting www.boost-consulting.com
In article <u64q4872w.fsf@boost-consulting.com>, David Abrahams <dave@boost-consulting.com> wrote:
Ben Artin <macdev@artins.org> writes:
I think that this particular nit is hardly worth the amount of discussion it's generated
Exactly.
I am happy to drop this, but I will give you my opinion, since I assume that you care enough to hear it once, and then just want everyone to move on to more important matters.
which is why Daryle and I believe that the change should be made now to make subsequent modifications easier.
Do you have any idea what code you're asking to have changed?
Yes.
Do you realize that the code being argued over was never checked in?
Yes.
The current test is (boost/python/detail/config.hpp):
# if !defined(_WIN32) && !defined(__CYGWIN__) \ && !defined(BOOST_PYTHON_USE_GCC_SYMBOL_VISIBILITY)
\ && BOOST_WORKAROUND(__GNUC__, >= 3) && (__GNUC_MINOR__ >=5 || __GNUC__ > 3) # define BOOST_PYTHON_USE_GCC_SYMBOL_VISIBILITY 1 # endif
Do you have a problem with that?
I do; that is *really* hard to read, IMO. It's clever, sure, but it's hard to read.
I can imagine that the following would be a very minor improvement, but it hardly seems to be worth the bits we're spending on it.
# if !defined(_WIN32) && !defined(__CYGWIN__) \ && !defined(BOOST_PYTHON_USE_GCC_SYMBOL_VISIBILITY)
\ && (BOOST_WORKAROUND(__GNUC__, == 3) && __GNUC_MINOR__ >=5 \ || BOOST_WORKAROUND(__GNUC__, > 3)) # define BOOST_PYTHON_USE_GCC_SYMBOL_VISIBILITY 1 # endif
It would be an improvement, it would be minor, I wouldn't have brought it up as I think it's a pretty small fish to fry, but Daryle did, and now that he has, my opinion is that he does have a good point and that the right thing is to make this code easier to read and (possibly) maintain. You are the maintainer, the final decision is yours, and I am going to turn my attention back to bigger things in my life. Ben -- I changed my name: <http://periodic-kingdom.org/People/NameChange.php>
Daryle Walker wrote:
On 11/29/05 4:27 PM, "Jason Kankiewicz" <jkankiewicz@advpubtech.com> wrote:
+# elif (defined(__GNUC__) && ((__GNUC__ >= 3 && __GNUC_MINOR__ >=5) || __GNUC__ >= 4))
Shouldn't the check for GCC 3.x be:
__GNUC__ == 3 && __GNUC_MINOR__ >= 5
instead of the currently over-broad version? Otherwise version numbers like 4.6 or 5.9 would match on that phrase, which is wrong.
I agree that your version's syntax is preferable. My only modification was to tack on "|| __GNUC__ >= 4" and the requisite enclosing parentheses in order to make the smallest change that could possibly work. Dave didn't apply my patch verbatim anyway as his fix is more elegant.
participants (21)
-
Ben Artin -
Daryle Walker -
David Abrahams -
Jason Kankiewicz