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

reorder forward declarations to get rid of C++14-only auto return types #2720

Merged
merged 41 commits into from Aug 11, 2017

Conversation

Projects
None yet
5 participants
@gentryx
Member

gentryx commented Jun 28, 2017

This fixes C++11-only builds (basically anything that's using CUDA). Closes #2718

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jun 28, 2017

Member

@gentryx This is roughly what we've had before. Unfortunately this fails compiling on some platforms (sorry, don't remember which) because of incomplete types. Could you please change the PR such that both versions of the code are there, using a pp constant to switch between the two (HPX_HAVE_CXX11? - I know this does not exist yet, it would need to be added here: https://github.com/STEllAR-GROUP/hpx/blob/master/cmake/HPX_DetectCppDialect.cmake)?

Member

hkaiser commented Jun 28, 2017

@gentryx This is roughly what we've had before. Unfortunately this fails compiling on some platforms (sorry, don't remember which) because of incomplete types. Could you please change the PR such that both versions of the code are there, using a pp constant to switch between the two (HPX_HAVE_CXX11? - I know this does not exist yet, it would need to be added here: https://github.com/STEllAR-GROUP/hpx/blob/master/cmake/HPX_DetectCppDialect.cmake)?

@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Jun 28, 2017

Member

@hkaiser Are you referring by "this" to the use of decltype instead of auto for the return types in the customization points? Yeah, I can change that to be configurable, no problem. :-)

Member

gentryx commented Jun 28, 2017

@hkaiser Are you referring by "this" to the use of decltype instead of auto for the return types in the customization points? Yeah, I can change that to be configurable, no problem. :-)

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jun 28, 2017

Member

@gentryx: I meant the whole forward declaration. Also, the use of auto without an explicit return type is intentional. So I'd appreciate it if you left the existing code unchanged as an alternative to the code you would like to submit.

Member

hkaiser commented Jun 28, 2017

@gentryx: I meant the whole forward declaration. Also, the use of auto without an explicit return type is intentional. So I'd appreciate it if you left the existing code unchanged as an alternative to the code you would like to submit.

@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Jun 28, 2017

Member

@hkaiser I think I don't quite understand. What I did was basically two things:

  1. I changed the C++14-style use of "auto" as a return type to be C++-11-compliant. I can change that to use the C++14-style if available.

  2. I rearranged the order of a ton of class/function definitions to make the C++11-style work (because that's referring to functions and classes that were previously defined after the customization points. This should not affect any platform, right? If it's working for my stricter builds, it should work on other platforms just as well. Or am I missing something?

Member

gentryx commented Jun 28, 2017

@hkaiser I think I don't quite understand. What I did was basically two things:

  1. I changed the C++14-style use of "auto" as a return type to be C++-11-compliant. I can change that to use the C++14-style if available.

  2. I rearranged the order of a ton of class/function definitions to make the C++11-style work (because that's referring to functions and classes that were previously defined after the customization points. This should not affect any platform, right? If it's working for my stricter builds, it should work on other platforms just as well. Or am I missing something?

@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Jun 28, 2017

Member

Oh, and I forgot:

  1. I removed the forward declaration for the customization points. Do you want me to re-add that to execution_fwd.hpp? I suppose I can do that as well, no problem.
Member

gentryx commented Jun 28, 2017

Oh, and I forgot:

  1. I removed the forward declaration for the customization points. Do you want me to re-add that to execution_fwd.hpp? I suppose I can do that as well, no problem.
@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jun 28, 2017

Member

@gentry:

  • In C++11 you can use just auto without explicit return type specification if the body of the function is just a single return statement.
  • The reordering you propose is exactly what we started off with but we had to change it to use the forward declaration as other compilers were complaining about incomplete types in C++14 mode
  • I'd like to ask you to (optionally) leave the code as it is right now and introduce your fix via a preprocessor constant which switches between the two implementations

Thanks!

Member

hkaiser commented Jun 28, 2017

@gentry:

  • In C++11 you can use just auto without explicit return type specification if the body of the function is just a single return statement.
  • The reordering you propose is exactly what we started off with but we had to change it to use the forward declaration as other compilers were complaining about incomplete types in C++14 mode
  • I'd like to ask you to (optionally) leave the code as it is right now and introduce your fix via a preprocessor constant which switches between the two implementations

Thanks!

@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Jun 28, 2017

Member

@hkaiser: I've added preprocessor guards so that the customization points will again use forward declarations and "auto" if C++14 is available, but will fall back to a C++11-compatible style if necessary.

I've successfully tested this build with GCC 4.9.4 with and without CUDA 8.0 for the C++11-mode and with GCC 6.3.0 for a C++14 build. Please let me know if you want me to try further compiler versions. :-)

Member

gentryx commented Jun 28, 2017

@hkaiser: I've added preprocessor guards so that the customization points will again use forward declarations and "auto" if C++14 is available, but will fall back to a C++11-compatible style if necessary.

I've successfully tested this build with GCC 4.9.4 with and without CUDA 8.0 for the C++11-mode and with GCC 6.3.0 for a C++14 build. Please let me know if you want me to try further compiler versions. :-)

@gentryx

Looks like clang on CircleCI is complaining. This is good because it allows me to debug the issue. I'm on it.

Show outdated Hide outdated cmake/tests/cxx14_auto.cpp
@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser
Member

hkaiser commented Jun 29, 2017

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jun 29, 2017

Member

should those changes be applied to the execution information and timed execution customization points as well: https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/parallel/executors/execution_information_fwd.hpp and https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/parallel/executors/timed_execution_fwd.hpp?

@gentryx: to clarify: you have applied different methods of resolving the issue at hand for the base execution customization points compared to how you resolved it for the timed execution and informational customization points. Why?

Also, the original code was using the auto return type deduction for functions on purpose (without explicit trailing return type). This improves error messages as the customization_point<>::operator()() implementations are not SFINAE'd out in case of some underlying problems. You propose to unconditionally replace this with explicit return type deductions (-> decltype(...)) for the customization points related to the timed execution and informational facilities. I understand that we have to add explicit return type deduction for some compilers, but we should leave the original code in place for conforming ones.

You have also removed the headers timed_execution_fwd.hpp and execution_information_fwd.hpp from the documentation without re-adding the corresponding headers (which now have the doxygen comments). This removes those customization points from the generated docs. A similar problem now occurs related to the doxygen comments for the main customization points. Those comments are now in a new header file which is not visible to the documentation tool chain, again removing them from the generated docs.

Member

hkaiser commented Jun 29, 2017

should those changes be applied to the execution information and timed execution customization points as well: https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/parallel/executors/execution_information_fwd.hpp and https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/parallel/executors/timed_execution_fwd.hpp?

@gentryx: to clarify: you have applied different methods of resolving the issue at hand for the base execution customization points compared to how you resolved it for the timed execution and informational customization points. Why?

Also, the original code was using the auto return type deduction for functions on purpose (without explicit trailing return type). This improves error messages as the customization_point<>::operator()() implementations are not SFINAE'd out in case of some underlying problems. You propose to unconditionally replace this with explicit return type deductions (-> decltype(...)) for the customization points related to the timed execution and informational facilities. I understand that we have to add explicit return type deduction for some compilers, but we should leave the original code in place for conforming ones.

You have also removed the headers timed_execution_fwd.hpp and execution_information_fwd.hpp from the documentation without re-adding the corresponding headers (which now have the doxygen comments). This removes those customization points from the generated docs. A similar problem now occurs related to the doxygen comments for the main customization points. Those comments are now in a new header file which is not visible to the documentation tool chain, again removing them from the generated docs.

@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Jun 30, 2017

Member

@hkaiser I added explicit return types (via decltype(...)) for customization_point because of NVCC's shortcomings WRT auto return types. This seems to be the only solution that works with NVCC (so apparently NVCC is not standard compliant). However, this is conditional on the CMake check for C++14 auto support, and if such support is found the code after the preprocessor is identical to the original code. As mentioned above I can change the conditional from "C++14 auto" to "C++11 auto".

If this approach is fine for you then I can follow the same pattern for timed_execution_fwd.hpp and execution_information_fwd.hpp. I had these headers removed as my builds (g++ 4.9.4 with CUDA 8.0 g++ 6.2.0 and clang 4.0) work just fine with moving those functions into the corresponding headers, but I hold no stake in this, I'm fine with doing the same as I did for execution.hpp/execution_fwd.hpp.

Member

gentryx commented Jun 30, 2017

@hkaiser I added explicit return types (via decltype(...)) for customization_point because of NVCC's shortcomings WRT auto return types. This seems to be the only solution that works with NVCC (so apparently NVCC is not standard compliant). However, this is conditional on the CMake check for C++14 auto support, and if such support is found the code after the preprocessor is identical to the original code. As mentioned above I can change the conditional from "C++14 auto" to "C++11 auto".

If this approach is fine for you then I can follow the same pattern for timed_execution_fwd.hpp and execution_information_fwd.hpp. I had these headers removed as my builds (g++ 4.9.4 with CUDA 8.0 g++ 6.2.0 and clang 4.0) work just fine with moving those functions into the corresponding headers, but I hold no stake in this, I'm fine with doing the same as I did for execution.hpp/execution_fwd.hpp.

@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Jun 30, 2017

Member

OK, looks like CircleCI is fine with the PR (the error it's throwing seems related to the Perl install?). :-)

Please advise on how to procedd WRT timed_execution_fwd.hpp/execution_information_fwd.hpp. Same pattern as for execution_fwd.hpp?

Member

gentryx commented Jun 30, 2017

OK, looks like CircleCI is fine with the PR (the error it's throwing seems related to the Perl install?). :-)

Please advise on how to procedd WRT timed_execution_fwd.hpp/execution_information_fwd.hpp. Same pattern as for execution_fwd.hpp?

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jun 30, 2017

Member

OK, looks like CircleCI is fine with the PR (the error it's throwing seems related to the Perl install?). :-)

No idea what's wrong there... I have never seen this before.

Please advise on how to procedd WRT timed_execution_fwd.hpp/execution_information_fwd.hpp. Same pattern as for execution_fwd.hpp?

Yes, please.

Member

hkaiser commented Jun 30, 2017

OK, looks like CircleCI is fine with the PR (the error it's throwing seems related to the Perl install?). :-)

No idea what's wrong there... I have never seen this before.

Please advise on how to procedd WRT timed_execution_fwd.hpp/execution_information_fwd.hpp. Same pattern as for execution_fwd.hpp?

Yes, please.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jul 7, 2017

Member

@gentryx: are you still working on this?

Member

hkaiser commented Jul 7, 2017

@gentryx: are you still working on this?

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 2, 2017

Member

K, I've refactored the patch for readability as you suggested.

Thanks, perfect!

The new CXX11 auto option is now off by default if HPX_WITH_CUDA is selected.

Uhh, shouldn't it be ON by default if CUDA is selected, but OFF by default otherwise?

Member

hkaiser commented Aug 2, 2017

K, I've refactored the patch for readability as you suggested.

Thanks, perfect!

The new CXX11 auto option is now off by default if HPX_WITH_CUDA is selected.

Uhh, shouldn't it be ON by default if CUDA is selected, but OFF by default otherwise?

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 10, 2017

Member

@gentryx: uhh, I moved this over to the branch https://github.com/STEllAR-GROUP/hpx/tree/gentryx-master in the main repo and fixed some things. I'll try to reintegrate your work with that branch tomorrow. Thanks!

Member

hkaiser commented Aug 10, 2017

@gentryx: uhh, I moved this over to the branch https://github.com/STEllAR-GROUP/hpx/tree/gentryx-master in the main repo and fixed some things. I'll try to reintegrate your work with that branch tomorrow. Thanks!

@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Aug 10, 2017

Member

@hkaiser Okidoki. I've just pushed some changes to please inspect. And yes: the option should be on by default and off if CUDA is being used as NVCC doesn't support it.

Member

gentryx commented Aug 10, 2017

@hkaiser Okidoki. I've just pushed some changes to please inspect. And yes: the option should be on by default and off if CUDA is being used as NVCC doesn't support it.

@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Aug 10, 2017

Member

@hkaiser Regarding your branch gentryx-master: the default value for the option needs to be inversed: on by default, off if CUDA (with gcc) is selected.

Member

gentryx commented Aug 10, 2017

@hkaiser Regarding your branch gentryx-master: the default value for the option needs to be inversed: on by default, off if CUDA (with gcc) is selected.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 10, 2017

Member

@gentry: ok, I'll verify again.

Member

hkaiser commented Aug 10, 2017

@gentry: ok, I'll verify again.

hkaiser added some commits Aug 8, 2017

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 10, 2017

Member

@gentryx I changed the default for the option (thanks for pointing this out) and reintegrated all of your recent changes into https://github.com/STEllAR-GROUP/hpx/tree/gentryx-master (please check, I hope I have not missed anything). Once circleci passes I'm going to merge things from there.

Member

hkaiser commented Aug 10, 2017

@gentryx I changed the default for the option (thanks for pointing this out) and reintegrated all of your recent changes into https://github.com/STEllAR-GROUP/hpx/tree/gentryx-master (please check, I hope I have not missed anything). Once circleci passes I'm going to merge things from there.

@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Aug 11, 2017

Member

@hkaiser Looks good so far. I've merged the gentryx-master branch in this PR and added another commit to mask some tests that won't compile in CXX11-mode (as they expect auto-typed lambda parameters).

Member

gentryx commented Aug 11, 2017

@hkaiser Looks good so far. I've merged the gentryx-master branch in this PR and added another commit to mask some tests that won't compile in CXX11-mode (as they expect auto-typed lambda parameters).

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 11, 2017

Member

@gentryx: thanks so much!

Member

hkaiser commented Aug 11, 2017

@gentryx: thanks so much!

@hkaiser

LGTM, thanks!

@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Aug 11, 2017

Member

I have no clue what the error message means that CircleCI is showing. Does this block the merge?

Member

gentryx commented Aug 11, 2017

I have no clue what the error message means that CircleCI is showing. Does this block the merge?

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 11, 2017

Member

@gentryx: I think this is happening because the build - for some reason - runs under your account, not under the STEllAR-GROUP account. I think we're fine merging anyways.

Member

hkaiser commented Aug 11, 2017

@gentryx: I think this is happening because the build - for some reason - runs under your account, not under the STEllAR-GROUP account. I think we're fine merging anyways.

@hkaiser hkaiser merged commit 130deb2 into STEllAR-GROUP:master Aug 11, 2017

1 of 2 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@gentryx

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Aug 11, 2017

Member

Woohoo! :-)

Member

gentryx commented Aug 11, 2017

Woohoo! :-)

@taeguk

This comment has been minimized.

Show comment
Hide comment
@taeguk

taeguk Aug 12, 2017

Member

@gentryx Excuse me, why did you do this? Are there CXX14 features in benchmark_partition and benchmark_partition_copy?? What are they?

Member

taeguk commented on fadd999 Aug 12, 2017

@gentryx Excuse me, why did you do this? Are there CXX14 features in benchmark_partition and benchmark_partition_copy?? What are they?

This comment has been minimized.

Show comment
Hide comment
@taeguk

taeguk Aug 12, 2017

Member

@gentryx I found it. It is reverted in #2820.

Member

taeguk replied Aug 12, 2017

@gentryx I found it. It is reverted in #2820.

This comment has been minimized.

Show comment
Hide comment
@gentryx

gentryx Aug 12, 2017

Member

Check. Thanks!

Member

gentryx replied Aug 12, 2017

Check. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment