Boost.Operators issue: polluting namespace of caller through argument-dependent lookup

newer
Removal of boost::basic_string_ref...

John M. Dlugosz

17 Jan 2016 17 Jan '16

9:51 a.m.

Here's the problem I ran into http://stackoverflow.com/questions/34449254/how-is-argument-dependent-lookup... |usingstd::tr1::shared_ptr; ||usingstd::tr1::make_shared;| |shared_ptr<Dog>Gamma::knockout (constHotel&target){Eggsoftboiled (target.ID);softboiled.sequence ^=0x40000000;returnmake_shared<Dog>(softboiled,target.kin,Frank::myself());} | The arguments to the constructor passed through make_shared are of types |Able::Bravo::Charlie::Egg&,constuint32_t&,constFrank&| The use of make_shared is finding boost::make_shared as well as std::make_shared. The program doesn't use boost::shared_ptr and shouldn't need to know about it. It turns out that Frank is derived privately from boost::totally_ordered1 (and and Egg from boost::totally ordered). Some Boost header that's being used in this translation unit just happens to pull in Boost's shared_ptr stuff. It ends up effectively polluting the namespace of possible calls involving these types, thanks to argument-dependent lookup. Furthermore, which Boost headers include which will change from version to version. I have this problem show up much more in Boost 1.60 than I have in Boost 1.59. There is no easy way around it beyond qualifying the call make_shared, and I specifically want to avoid that so I don't litter the program with tr1's (this varies by compiler and version as to whether tr1 is required, prohibited, or optional, so I want it in one place.) I believe the proper thing to do, which is a very simple fix actually, is to define the mix-in classes like totally_ordered (and other things from Boost.Operators in particular) in a unique inner namespace, and then make aliases in the boost namespace where they are published as being found. namespace boost { *namespace inner_name_1 { * template partially_ordered { ⋯ }; // the entire guts of Boost.Operators can go here, I think. *} // end of inner isolating namespace** ** ** using inner_name_1::partially_ordered;* // etc. } This inner isolation technique should be SOP for any classes that are meant to be inherited from by user (non-Boost library) code. It would not hurt to also isolate classes that match the names of std library code, in general! This would, in general, prevent the libraries from interfering with each other and surprises popping up when using std components and pulling in Boost headers. —John ||

Show replies by date

Daniel Frey

18 Jan 18 Jan

6:03 p.m.

New subject: Boost.Operators issue: polluting namespace of caller through argument-dependent lookup

...

On 17.01.2016, at 10:51, John M. Dlugosz wrote:

Here's the problem I ran into http://stackoverflow.com/questions/34449254/how-is-argument-dependent-lookup...

|usingstd::tr1::shared_ptr; ||usingstd::tr1::make_shared;|

|shared_ptr<Dog>Gamma::knockout (constHotel&target){Eggsoftboiled (target.ID);softboiled.sequence ^=0x40000000;returnmake_shared<Dog>(softboiled,target.kin,Frank::myself());} |

The arguments to the constructor passed through make_shared are of types

|Able::Bravo::Charlie::Egg&,constuint32_t&,constFrank&|

The use of make_shared is finding boost::make_shared as well as std::make_shared. The program doesn't use boost::shared_ptr and shouldn't need to know about it.

This can not be simply “fixed” as it is quite an old and established interface. What about the people that actually rely on ADL to kick in to find their unqualified call of make_shared? Ideally, this should have been discussed and prevented when those interfaces were first established and I used the exact same solution that you described for a (Boost-independant) rewrite I did some years ago, but for Boost.Operators I think it is too late. (unless you can convince *everyone* that it should be changed, but I can not see how that is possible). This leaves you only one option: Don’t use unqualified calls in your code unless it is actually required - which does not seem to be the case in your example. This is a general guideline I follow as it prevents these ADL-related problems. Regards, Daniel

Andrey Semashev

7:11 p.m.

On 2016-01-18 21:03, Daniel Frey wrote:

...

...
On 17.01.2016, at 10:51, John M. Dlugosz wrote:

The use of make_shared is finding boost::make_shared as well as std::make_shared. The program doesn't use boost::shared_ptr and shouldn't need to know about it.

This can not be simply “fixed” as it is quite an old and established interface. What about the people that actually rely on ADL to kick in to find their unqualified call of make_shared?

Given that there is BOOST_NO_OPERATORS_IN_NAMESPACE, I don't think that the addition of the boost namespace to ADL was ever intended. I would rather see this fixed and the code that relies on this side effect broken (with the appropriate note in the release notes and the advice to use namespace qualification to resolve the problem).

Andrey Semashev

7:15 p.m.

On 2016-01-18 22:11, Andrey Semashev wrote:

...

On 2016-01-18 21:03, Daniel Frey wrote:

...
...
On 17.01.2016, at 10:51, John M. Dlugosz wrote:

The use of make_shared is finding boost::make_shared as well as std::make_shared. The program doesn't use boost::shared_ptr and shouldn't need to know about it.

This can not be simply “fixed” as it is quite an old and established interface. What about the people that actually rely on ADL to kick in to find their unqualified call of make_shared?

Given that there is BOOST_NO_OPERATORS_IN_NAMESPACE, I don't think that the addition of the boost namespace to ADL was ever intended. I would rather see this fixed and the code that relies on this side effect broken (with the appropriate note in the release notes and the advice to use namespace qualification to resolve the problem).

I forgot to mention there was a similar issue with Boost.Iterator not long ago, which was resolved the similar way.

Daniel Frey

7:36 p.m.

New subject: Boost.Operators issue: polluting namespace of caller through argument-dependent lookup

...

On 18.01.2016, at 20:11, Andrey Semashev wrote:

On 2016-01-18 21:03, Daniel Frey wrote:

...
...
On 17.01.2016, at 10:51, John M. Dlugosz wrote:

The use of make_shared is finding boost::make_shared as well as std::make_shared. The program doesn't use boost::shared_ptr and shouldn't need to know about it.

This can not be simply “fixed” as it is quite an old and established interface. What about the people that actually rely on ADL to kick in to find their unqualified call of make_shared?

Given that there is BOOST_NO_OPERATORS_IN_NAMESPACE, I don't think that the addition of the boost namespace to ADL was ever intended. I would rather see this fixed and the code that relies on this side effect broken (with the appropriate note in the release notes and the advice to use namespace qualification to resolve the problem).

I think that BOOST_NO_OPERATORS_IN_NAMESPACE is meant to put everything into the global namespace, at least that’s what the comment in Boost.Operators is saying. This is quite different from putting them into an additional nested namespace (e.g. operator_impl) and adding a “using namespace operator_impl;” into namespace boost. And from what I remember, this was required as a work-around for bugs in some very old compilers. That said, I sympathize with fixing the issue when possible. Maybe we could change the default behavior, but offer the old implementation when the user sets BOOST_PLACE_OPERATORS_IN_NAMESPACE_BOOST. And that is only effective when BOOST_NO_OPERATORS_IN_NAMESPACE is *not* used (to avoid breaking the old compilers). Opinions? Regards, Daniel

Andrey Semashev

7:47 p.m.

On 2016-01-18 22:36, Daniel Frey wrote:

...

...
On 18.01.2016, at 20:11, Andrey Semashev wrote:

On 2016-01-18 21:03, Daniel Frey wrote:

...
...
On 17.01.2016, at 10:51, John M. Dlugosz wrote:

The use of make_shared is finding boost::make_shared as well as std::make_shared. The program doesn't use boost::shared_ptr and shouldn't need to know about it.

This can not be simply “fixed” as it is quite an old and established interface. What about the people that actually rely on ADL to kick in to find their unqualified call of make_shared?

Given that there is BOOST_NO_OPERATORS_IN_NAMESPACE, I don't think that the addition of the boost namespace to ADL was ever intended. I would rather see this fixed and the code that relies on this side effect broken (with the appropriate note in the release notes and the advice to use namespace qualification to resolve the problem).

I think that BOOST_NO_OPERATORS_IN_NAMESPACE is meant to put everything into the global namespace, at least that’s what the comment in Boost.Operators is saying. This is quite different from putting them into an additional nested namespace (e.g. operator_impl) and adding a “using namespace operator_impl;” into namespace boost. And from what I remember, this was required as a work-around for bugs in some very old compilers.

The point is that the boost namespace is not guaranteed to be introduced to ADL, so one can't rely on that.

...

That said, I sympathize with fixing the issue when possible. Maybe we could change the default behavior, but offer the old implementation when the user sets BOOST_PLACE_OPERATORS_IN_NAMESPACE_BOOST. And that is only effective when BOOST_NO_OPERATORS_IN_NAMESPACE is *not* used (to avoid breaking the old compilers). Opinions?

Sounds good to me.

Daniel Frey

15 Feb 15 Feb

4:58 p.m.

New subject: Ping to the admins (was Re: Boost.Operators issue: polluting namespace of caller through argument-dependent lookup)

...

On 18.01.2016, at 20:47, Andrey Semashev wrote:

On 2016-01-18 22:36, Daniel Frey wrote:

...
That said, I sympathize with fixing the issue when possible. Maybe we could change the default behavior, but offer the old implementation when the user sets BOOST_PLACE_OPERATORS_IN_NAMESPACE_BOOST. And that is only effective when BOOST_NO_OPERATORS_IN_NAMESPACE is *not* used (to avoid breaking the old compilers). Opinions?

Sounds good to me.

I’d like to actually work on this, but my ticket to get access to Boost.Utility is just sitting idle… https://github.com/boostorg/admin/issues/111 If the above ticket is not the right way to move forward, please someone let me know what to do instead. Thanks.

Rene Rivera

17 Feb 17 Feb

4:33 p.m.

New subject: Ping to the admins (was Re: Boost.Operators issue: polluting namespace of caller through argument-dependent lookup)

Sorry about the delay.. You should now have an invite for this. On Mon, Feb 15, 2016 at 10:58 AM, Daniel Frey wrote:

...

...
On 18.01.2016, at 20:47, Andrey Semashev wrote:

On 2016-01-18 22:36, Daniel Frey wrote:

...
That said, I sympathize with fixing the issue when possible. Maybe we could change the default behavior, but offer the old implementation when the user sets BOOST_PLACE_OPERATORS_IN_NAMESPACE_BOOST. And that is only effective when BOOST_NO_OPERATORS_IN_NAMESPACE is *not* used (to avoid breaking the old compilers). Opinions?

Sounds good to me.

I’d like to actually work on this, but my ticket to get access to Boost.Utility is just sitting idle…

https://github.com/boostorg/admin/issues/111

If the above ticket is not the right way to move forward, please someone let me know what to do instead. Thanks.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

-- -- Rene Rivera -- Grafik - Don't Assume Anything -- Robot Dreams - http://robot-dreams.net -- rrivera/acm.org (msn) - grafikrobot/aim,yahoo,skype,efnet,gmail

Daniel Frey

5:57 p.m.

New subject: Ping to the admins (was Re: Boost.Operators issue: polluting namespace of caller through argument-dependent lookup)

...

On 17.02.2016, at 17:33, Rene Rivera wrote:

Sorry about the delay.. You should now have an invite for this.

Thanks, seems to work. On the topic of fixing the issue reported: I realize that is_chained_base is actually a PITA when you try to add another namespace around the operator templates. I’ll have to think of a solution. Just a heads up for others: is_chained_base was placed in ::boost, not ::boost::detail. I assume this is an oversight as the documentation does not mention it and a comment in the source is implying that it should actually should have been placed in ::boost::detail. If anyone ever used it as part of the public interface, please speak up now! :)

...

On Mon, Feb 15, 2016 at 10:58 AM, Daniel Frey wrote:

...
...
On 18.01.2016, at 20:47, Andrey Semashev wrote:

On 2016-01-18 22:36, Daniel Frey wrote:

...
That said, I sympathize with fixing the issue when possible. Maybe we could change the default behavior, but offer the old implementation when the user sets BOOST_PLACE_OPERATORS_IN_NAMESPACE_BOOST. And that is only effective when BOOST_NO_OPERATORS_IN_NAMESPACE is *not* used (to avoid breaking the old compilers). Opinions?

Sounds good to me.

I’d like to actually work on this, but my ticket to get access to Boost.Utility is just sitting idle…

https://github.com/boostorg/admin/issues/111

If the above ticket is not the right way to move forward, please someone let me know what to do instead. Thanks.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

-- -- Rene Rivera -- Grafik - Don't Assume Anything -- Robot Dreams - http://robot-dreams.net -- rrivera/acm.org (msn) - grafikrobot/aim,yahoo,skype,efnet,gmail

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Andrey Semashev

6:05 p.m.

New subject: Ping to the admins (was Re: Boost.Operators issue: polluting namespace of caller through argument-dependent lookup)

On 2016-02-17 20:57, Daniel Frey wrote:

...

On the topic of fixing the issue reported: I realize that is_chained_base is actually a PITA when you try to add another namespace around the operator templates. I’ll have to think of a solution. Just a heads up for others: is_chained_base was placed in ::boost, not ::boost::detail. I assume this is an oversight as the documentation does not mention it and a comment in the source is implying that it should actually should have been placed in ::boost::detail. If anyone ever used it as part of the public interface, please speak up now! :)

Not sure about the purpose of is_chained_base, but might I suggest avoid moving it into boost::detail. Bringing in 'boost::detail' into ADL is almost as bad as bringing in 'boost'. This also concerns other types that are used in template parameters of operator base classes.

Daniel Frey

6:20 p.m.

New subject: Ping to the admins (was Re: Boost.Operators issue: polluting namespace of caller through argument-dependent lookup)

...

On 17.02.2016, at 19:05, Andrey Semashev wrote:

On 2016-02-17 20:57, Daniel Frey wrote:

...
On the topic of fixing the issue reported: I realize that is_chained_base is actually a PITA when you try to add another namespace around the operator templates. I’ll have to think of a solution. Just a heads up for others: is_chained_base was placed in ::boost, not ::boost::detail. I assume this is an oversight as the documentation does not mention it and a comment in the source is implying that it should actually should have been placed in ::boost::detail. If anyone ever used it as part of the public interface, please speak up now! :)

Not sure about the purpose of is_chained_base, but might I suggest avoid moving it into boost::detail. Bringing in 'boost::detail' into ADL is almost as bad as bringing in 'boost'. This also concerns other types that are used in template parameters of operator base classes.

I wasn’t planning on moving it to boost::detail, I will probably rename boost::detail to boost::operators_impl (within operators.hpp) anyways for the reasons you mentioned. I was only wondering if a can assume is_chained_base to be an implementation detail or if anyone thought that it’s part of the public interface.

John M. Dlugosz

19 Jan 19 Jan

11:43 p.m.

On 1/18/2016 1:11 PM, Andrey Semashev wrote:

...

On 2016-01-18 21:03, Daniel Frey wrote:

...
...
On 17.01.2016, at 10:51, John M. Dlugosz wrote:

The use of make_shared is finding boost::make_shared as well as std::make_shared. The program doesn't use boost::shared_ptr and shouldn't need to know about it.

This can not be simply fixed as it is quite an old and established interface. What about the people that actually rely on ADL to kick in to find their unqualified call of make_shared?

Given that there is BOOST_NO_OPERATORS_IN_NAMESPACE, I don't think that the addition of the boost namespace to ADL was ever intended. I would rather see this fixed and the code that relies on this side effect broken (with the appropriate note in the release notes and the advice to use namespace qualification to resolve the problem).

It already varies with the version of Boost! And with the selected headers being pulled in by other code. So it already is a wild card that might work in some places and not others. make_shared (or any other pre-standard library) is not related to the Operators feature, and people shouldn't count on it interacting in an undocumented way. And it doesn't really: it depends on what else is included.

Daniel Frey

20 Jan 20 Jan

4:41 p.m.

New subject: Boost.Operators issue: polluting namespace of caller through argument-dependent lookup

...

On 20.01.2016, at 00:43, John M. Dlugosz wrote:

On 1/18/2016 1:11 PM, Andrey Semashev wrote:

...
On 2016-01-18 21:03, Daniel Frey wrote:

...
...
On 17.01.2016, at 10:51, John M. Dlugosz wrote:

The use of make_shared is finding boost::make_shared as well as std::make_shared. The program doesn't use boost::shared_ptr and shouldn't need to know about it.

This can not be simply fixed as it is quite an old and established interface. What about the people that actually rely on ADL to kick in to find their unqualified call of make_shared?

Given that there is BOOST_NO_OPERATORS_IN_NAMESPACE, I don't think that the addition of the boost namespace to ADL was ever intended. I would rather see this fixed and the code that relies on this side effect broken (with the appropriate note in the release notes and the advice to use namespace qualification to resolve the problem).

It already varies with the version of Boost! And with the selected headers being pulled in by other code. So it already is a wild card that might work in some places and not others.

It won’t vary when you are doing it on purpose, you’d include the necessary headers explicitly instead of relying on some side-effect. Only when you are using unqualified lookup and you don’t expect this, it might change with the version of Boost.

...

make_shared (or any other pre-standard library) is not related to the Operators feature, and people shouldn't count on it interacting in an undocumented way. And it doesn't really: it depends on what else is included.

In a way it makes library collections like Boost harder and I still think you should use qualified lookup in your specific case, but as I already said in another mail I’m willing to fix it. I just need to figure out where to commit and merge stuff as the last time I worked on it we were still using svn and the branching model was different. I hope to be able to find the time somewhere in the next days. Regards, Daniel

3205

Age (days ago)

3236

Last active (days ago)

List overview

Download

12 comments

4 participants

participants (4)

Andrey Semashev
Daniel Frey
John M. Dlugosz
Rene Rivera