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

Add missing includes #112

Merged
merged 2 commits into from
Dec 3, 2023
Merged

Conversation

ashtum
Copy link
Collaborator

@ashtum ashtum commented Nov 15, 2023

No description provided.

@ashtum ashtum marked this pull request as draft November 15, 2023 14:26
@pdimov
Copy link
Member

pdimov commented Nov 15, 2023

It's hard to say what's the history here.

I suppose this dates from the days when including <boost/bind.hpp> made _1 et al available in the global namespace, which later conflicted with using namespace std::placeholders; which also made _1 available.

So I assume people who were defining BOOST_BIND_NO_PLACEHOLDERS did that in order to be able to use using namespace std::placeholders; in their code. But doing so broke parser.hpp.

This problem will not occur today, because <boost/bind/bind.hpp> leaves the placeholders defined in namespace boost::placeholders, avoiding the conflict. But for backward compatibility, we may want to still support the configuration with BOOST_BIND_NO_PLACEHOLDERS defined.

So adding this include is probably correct, but we need a test that shows why.

@pdimov
Copy link
Member

pdimov commented Nov 15, 2023

As a general rule, every change that does not immediately fix a test failure needs to follow the procedure of: first commit adds a test that fails without the change, second commit adds the change that fixes the test failure.

@ashtum ashtum force-pushed the add-missing-includes branch 2 times, most recently from b5571f9 to 5210596 Compare November 16, 2023 10:02
test/Jamfile.v2 Outdated Show resolved Hide resolved
@pdimov
Copy link
Member

pdimov commented Nov 16, 2023

Would you please be able to extract the new test into a separate PR? I'd like to merge the original contribution (#101) as a form of giving credit on top of it.

@ashtum ashtum force-pushed the add-missing-includes branch from 5210596 to ce6c489 Compare November 16, 2023 12:53
@ashtum
Copy link
Collaborator Author

ashtum commented Nov 16, 2023

Then this PR is waiting for #100 response.

@ashtum ashtum force-pushed the add-missing-includes branch from ce6c489 to 76fe0d5 Compare November 29, 2023 05:48
@ashtum ashtum marked this pull request as ready for review November 29, 2023 07:39
@ashtum ashtum force-pushed the add-missing-includes branch from 843349e to b7692c2 Compare November 29, 2023 14:47
@ashtum ashtum requested a review from pdimov November 29, 2023 15:20
@pdimov pdimov merged commit ddaf922 into boostorg:develop Dec 3, 2023
55 checks passed
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.

2 participants