boostorg / phoenix

Boost.org phoenix module
http://boost.org/libs/phoenix
30 stars 46 forks source link

Partial Revert "std::tuple support (Resolving #103) (#104)" #112

Open sgn opened 1 year ago

sgn commented 1 year ago

This partial reverts commit 8b6a9c26c115bc2cefea300b5c0abf7edf6dd9b7.

The being-reverted-change put uarg{1..10} in all translation unit that include "boost/phoenix/stl.hpp", should that file is being included by multiple translation unit, each of those translation units will have one definition of each of uarg{1..10}. Thus, we'll run into below error:

multiple definition of `boost::phoenix::placeholders::uarg1'

Partial Revert the change in question by removing "boost/phoenix/stl/tuple.hpp" from "boost/phoenix/stl.hpp".

Fix #111

sgn commented 1 year ago

@beojan

sgn commented 1 year ago

Since we're in bug-fix period for 1.81.0, I wouldn't want to revert a whole PR, which someone maybe rely on already

beojan commented 1 year ago

@sgn Does changing the uargX definition in stl/tuple.hpp to be const work?

That's on line 113.

sgn commented 1 year ago

With g++-12 -std=c++14 const yes, constexpr no. I'm not a language lawyer hence I'm not sure what standard would say about this. If you can quote something from standard that make const legal, I would happy to change the PR, I won't want to have a broken release for some compilers.

% cat test.cpp                                                                                       
#include <boost/phoenix/stl/tuple.hpp>

int func();

int main() {
        return func();
}
@selene /tmp 
% cat other.cpp                                                                                      
#include <boost/phoenix/stl/tuple.hpp>

int func();

int func() {
        return 0;
}
@selene /tmp 
% g++ -fPIC -std=c++14 -I /home/boost/include test.cpp other.cpp -o abc
--- boost/phoenix/stl/tuple.hpp.orig    2022-11-29 00:42:17.884124579 +0700
+++ boost/phoenix/stl/tuple.hpp 2022-11-29 00:40:35.163960550 +0700
@@ -110,7 +110,7 @@
     namespace placeholders {
         #define BOOST_PP_LOCAL_LIMITS (1, BOOST_PHOENIX_ARG_LIMIT)
         #define BOOST_PP_LOCAL_MACRO(N)                                                \
-            auto uarg##N =                                                             \
+            const auto uarg##N =                                                             \
             boost::phoenix::get_<(N)-1>(boost::phoenix::placeholders::arg1);
         #include BOOST_PP_LOCAL_ITERATE()
     }
beojan commented 1 year ago

const is what's used for the argX placeholders, which is why this problem didn't show up with those.

I think the reason this works is that const variables have internal linkage so they can't be referred to from other translation units.

djowel commented 1 year ago

The checks are failing :-(

sgn commented 1 year ago

OK, I changed it into internal linkage.

ytimenkov commented 1 year ago

OOC: what is the problem with constexpr?

sgn commented 1 year ago

OOC: what is the problem with constexpr?

./boost/phoenix/stl/tuple.hpp:114:40: error: call to non-'constexpr' function 'const typename boost::phoenix::expression::get_with_idx<boost::phoenix::tuple_detail::idxwrap, Tuple>::type boost::phoenix::get(const Tuple&) [with int N = 1; Tuple = actor<boost::proto::exprns_::basicexpr<boost::proto::tagns::tag::terminal, boost::proto::argsns_::term<argument<1> >, 0> >; typename expression::get_with_idx<tuple_detail::idxwrap, Tuple>::type = actor<boost::proto::exprns::basic_expr<tag::get_withidx, boost::proto::argsns::list2<boost::proto::exprns_::basicexpr<boost::proto::tagns::tag::terminal, boost::proto::argsns_::term<tuple_detail::idxwrap<1> >, 0>, actor<boost::proto::exprns::basicexpr<boost::proto::tagns::tag::terminal, boost::proto::argsns::term<argument<1> >, 0> > >, 2> >]' 114 | boost::phoenix::get<(N)-1>(boost::phoenix::placeholders::arg1);

ytimenkov commented 1 year ago

Ah, I see... So it needs to be constexpr all the way... And inline for variable is C++17 only... I worked this around by defining BOOST_PHOENIX_STL_TUPLE_H_ :roll_eyes: . Trying to include only bind.hpp which is in use didn't compile either :(

beojan commented 1 year ago

Ah, I see... So it needs to be constexpr all the way... And inline for variable is C++17 only... I worked this around by defining BOOST_PHOENIX_STL_TUPLE_H_ roll_eyes . Trying to include only bind.hpp which is in use didn't compile either :(

Did you have an issue with making the uarg definitions const, which @sgn reported works?

djowel commented 1 year ago

No constexpr please. Phoenix is supposed to work with older compilers.

beojan commented 1 year ago

As it stands this PR doesn't use constexpr (only const) so that should be fine.

killerbot242 commented 1 year ago

has this been fixed ? Seems the release of 1.81 contains this problem,and breaks our code base ?

djowel commented 1 year ago

Can we please fix this? Perhaps we should just revert to the latest stable state.

beojan commented 1 year ago

Are the failed CI checks connected to the contents of this PR at all?

sgn commented 1 year ago

Are the failed CI checks connected to the contents of this PR at all?

Same error:

./boost/container_hash/is_described_class.hpp:10: fatal error: boost/describe/bases.hpp: No such file or directory

I think no, they ain't connected.

fkonvick commented 1 year ago

This happened to me with MSVC 2019 and Boost 1.81.0. Including <boost/phoenix/stl.hpp> in multiple translation units resulted in multiple symbol definitions. I was able to work around by replacing the original include in my .cpp with

#include <boost/phoenix/stl/algorithm.hpp>
#include <boost/phoenix/stl/container.hpp>