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

Fix let assigned from construct from array #61

Merged

Conversation

Kojoley
Copy link
Contributor

@Kojoley Kojoley commented Jan 2, 2018

Fixes #58 (comment) problem.

Note: let(_a = construct<T[N]>(...)) is an invalid thing because you cannot return arrays. Some static assert may be placed for this usage.

@Kojoley Kojoley force-pushed the fix-let-assigned-from-construct-from-array branch 2 times, most recently from af47816 to d714071 Compare January 3, 2018 13:27
@Kojoley
Copy link
Contributor Author

Kojoley commented Jan 3, 2018

Disclaimer: I do not know if A can be a reference type in this context (actually I tried to remove proto::detail::uncvref and it was fine), but I remember that some old compilers are not fine with A const& if A is already a reference (and it should be done with add_const + add_reference), but at least all tests passes on msvc-10.

@Flast
Copy link
Collaborator

Flast commented Jan 24, 2018

Closing and reopening to re-create travis job.

@Flast Flast closed this Jan 24, 2018
@Flast Flast reopened this Jan 24, 2018
@Kojoley Kojoley force-pushed the fix-let-assigned-from-construct-from-array branch from d714071 to 49a4618 Compare January 25, 2018 00:06
@Kojoley
Copy link
Contributor Author

Kojoley commented Jan 25, 2018

I have updated PR with add_const + add_reference for msvc-8.

P.S. Is it normal that I had to add -DBOOST_PHOENIX_NO_VARIADIC_EXPRESSION to wave.cfg to generate the preprocessed files?

@Flast
Copy link
Collaborator

Flast commented Jan 26, 2018

IMO, add proxy traits to boost/phoenix/config.hpp and switch its impl with BOOST_WORKAROUND, then use the proxy traits is better way to support buggy compiler.

Anyway, PP-ing is hard for somebody (especially newbie contributor), so defining BOOST_PHOENIX_DONT_USE_PREPROCESSED_FILES while CI is helpful (I'll do).

@Kojoley
Copy link
Contributor Author

Kojoley commented Jan 26, 2018

Actually it is not only msvc-8 problem, gcc in c++03 mode complains too https://travis-ci.org/boostorg/phoenix/builds/332629779

Anyway, PP-ing is hard for somebody (especially newbie contributor), so defining BOOST_PHOENIX_DONT_USE_PREPROCESSED_FILES while CI is helpful (I'll do).

I do not see how it will fix problem that currently wave preprocessing goes with the wrong settings and that's why does not generate any output.

@Kojoley
Copy link
Contributor Author

Kojoley commented Jan 26, 2018

I suspect the mac builds will take forever to end. You might want to restart them or, better, turn them off.

@Flast
Copy link
Collaborator

Flast commented Feb 7, 2018

Sorry for review delaying...

IMO, add proxy traits to boost/phoenix/config.hpp and switch its impl with BOOST_WORKAROUND, then use the proxy traits is better way to support buggy compiler.

Oops, I mistaken you said for it needs two form A0 and add_ref...<A0>::type separately. OK, your patch seems good.

P.S. Is it normal that I had to add -DBOOST_PHOENIX_NO_VARIADIC_EXPRESSION to wave.cfg to generate the preprocessed files?

AFAIK, wave should be executed without c++11 feature flag, i.e. the macro should be defined implicitly.

Anyway, PP-ing is hard for somebody (especially newbie contributor), so defining BOOST_PHOENIX_DONT_USE_PREPROCESSED_FILES while CI is helpful (I'll do).

I think requesting PP-ing inhibits developing cycle (especially newbie developer). Additionally, PP-ed PR is sometimes huge and makes human unreadable (hard to review) one... So, disabling use of PP-ed code while CI, makes contributor don't have to worry about that, is what I meant on that line.

@Flast Flast merged commit 389b204 into boostorg:develop Feb 7, 2018
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.

None yet

2 participants