Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

sgn
Copy link

@sgn sgn commented Nov 27, 2022

This partial reverts commit 8b6a9c2.

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
Copy link
Author

sgn commented Nov 27, 2022

@beojan

@sgn
Copy link
Author

sgn commented Nov 27, 2022

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
Copy link
Contributor

beojan commented Nov 28, 2022

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

That's on line 113.

@sgn
Copy link
Author

sgn commented Nov 28, 2022

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
Copy link
Contributor

beojan commented Nov 28, 2022

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
Copy link
Collaborator

djowel commented Nov 29, 2022

The checks are failing :-(

@sgn
Copy link
Author

sgn commented Nov 29, 2022

OK, I changed it into internal linkage.

@ytimenkov
Copy link

OOC: what is the problem with constexpr?

@sgn
Copy link
Author

sgn commented Nov 30, 2022

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::idx_wrap, Tuple>::type boost::phoenix::get_(const Tuple&) [with int N = 1; Tuple = actor<boost::proto::exprns_::basic_expr<boost::proto::tagns_::tag::terminal, boost::proto::argsns_::term<argument<1> >, 0> >; typename expression::get_with_idx<tuple_detail::idx_wrap, Tuple>::type = actor<boost::proto::exprns_::basic_expr<tag::get_with_idx, boost::proto::argsns_::list2<boost::proto::exprns_::basic_expr<boost::proto::tagns_::tag::terminal, boost::proto::argsns_::term<tuple_detail::idx_wrap<1> >, 0>, actor<boost::proto::exprns_::basic_expr<boost::proto::tagns_::tag::terminal, boost::proto::argsns_::term<argument<1> >, 0> > >, 2> >]' 114 | boost::phoenix::get_<(N)-1>(boost::phoenix::placeholders::arg1);

@ytimenkov
Copy link

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_ 🙄 . Trying to include only bind.hpp which is in use didn't compile either :(

@beojan
Copy link
Contributor

beojan commented Nov 30, 2022

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
Copy link
Collaborator

djowel commented Dec 1, 2022

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

@beojan
Copy link
Contributor

beojan commented Dec 1, 2022

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

@killerbot242
Copy link

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

@djowel
Copy link
Collaborator

djowel commented Dec 25, 2022

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

@beojan
Copy link
Contributor

beojan commented Dec 25, 2022

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

@sgn
Copy link
Author

sgn commented Dec 25, 2022

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.

8b6a9c2 (std::tuple support (Resolving boostorg#103) (boostorg#104), 2021-03-11) put
uarg##N in all translation units, which includes
boost/phoenix/stl/tuple.hpp or boost/phoenix/stl.hpp, with external
linkage. Thus, we'll run into below error:

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

Change it to internal linkage.
@fkonvick
Copy link

fkonvick commented Feb 8, 2023

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>

pld-gitsync pushed a commit to pld-linux/boost that referenced this pull request Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

boost 1.8.1 beta1+gcc-12: multiple definition of `boost::phoenix::placeholders::uarg1'
6 participants