Boost.Operators issue: polluting namespace of caller through argument-dependent lookup
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<typename T, Typename U> 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 ||
On 17.01.2016, at 10:51, John M. Dlugosz <mpbecey7gu@snkmail.com> 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
On 2016-01-18 21:03, Daniel Frey wrote:
On 17.01.2016, at 10:51, John M. Dlugosz <mpbecey7gu@snkmail.com> 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).
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 <mpbecey7gu@snkmail.com> 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.
On 18.01.2016, at 20:11, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On 2016-01-18 21:03, Daniel Frey wrote:
On 17.01.2016, at 10:51, John M. Dlugosz <mpbecey7gu@snkmail.com> 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
On 2016-01-18 22:36, Daniel Frey wrote:
On 18.01.2016, at 20:11, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On 2016-01-18 21:03, Daniel Frey wrote:
On 17.01.2016, at 10:51, John M. Dlugosz <mpbecey7gu@snkmail.com> 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.
On 18.01.2016, at 20:47, Andrey Semashev <andrey.semashev@gmail.com> 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.
Sorry about the delay.. You should now have an invite for this. On Mon, Feb 15, 2016 at 10:58 AM, Daniel Frey <d.frey@gmx.de> wrote:
On 18.01.2016, at 20:47, Andrey Semashev <andrey.semashev@gmail.com> 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
On 17.02.2016, at 17:33, Rene Rivera <grafikrobot@gmail.com> 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 <d.frey@gmx.de> wrote:
On 18.01.2016, at 20:47, Andrey Semashev <andrey.semashev@gmail.com> 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
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.
On 17.02.2016, at 19:05, Andrey Semashev <andrey.semashev@gmail.com> 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.
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 <mpbecey7gu@snkmail.com> 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.
On 20.01.2016, at 00:43, John M. Dlugosz <mpbecey7gu@snkmail.com> 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 <mpbecey7gu@snkmail.com> 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
participants (4)
-
Andrey Semashev -
Daniel Frey -
John M. Dlugosz -
Rene Rivera