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

shouldn't "-faligned-new" be a usage requirement? #3863

Closed
albestro opened this issue May 16, 2019 · 4 comments · Fixed by #3865
Closed

shouldn't "-faligned-new" be a usage requirement? #3863

albestro opened this issue May 16, 2019 · 4 comments · Fixed by #3865
Milestone

Comments

@albestro
Copy link
Contributor

albestro commented May 16, 2019

Including HPX libraries from my project moves the -faligned-new problem to my build.

I think that it is an HPX-related thing so it may be better to set this flag as usage requirement.

@hkaiser it's actually unrelated to nvcc. It's the newer gcc combined with C++11 that's complaining about it. I'd suggest adding hpx_add_compile_flag_if_available(-faligned-new) since it's good to have it enabled outside of just C++17. Alternatively hpx_add_compile_flag_if_available(-Wno-aligned-new) (I think that would work), since it's a rather benign warning.

Originally posted by @msimberg in #3745 (comment)

@msimberg msimberg added this to the 1.3.0 milestone May 16, 2019
@msimberg
Copy link
Contributor

I'd most likely add the following:

if(CMAKE_COMPILE_IS_GNUCXX AND NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 7.0)
  hpx_add_target_compile_option(-faligned-new PUBLIC)
endif()

Incidentally, we also need to move the place where we do this because the flag is added inside an if(HPX_WITH_COMPILER_WARNINGS) block.

We could also add another compile test that checks for aligned new, not just alignas. I wonder what older GCCs actually do with over-aligned news.

@albestro
Copy link
Contributor Author

Since the flag -faligned-new is not available in all compiler, but just in new ones partially supporting some new C++17 functionality, I think that this flag can be considered "nice to have, but optional" for what concerns HPX functionalities.
Moreover, this idea is supported by the fact that at the moment is added just if warnings are enabled and iff it is available (obviously) in the compiler used.

What I think is that it may be considered as an option and not a warning, so as you also suggested I would just move it out from the block as any other flag about compiler options (e.g. -Xclang -fcoroutines-ts PUBLIC).

@hkaiser
Copy link
Member

hkaiser commented May 16, 2019

@msimberg I think we should turn this into a feature test. For instance, MSVC supports over-aligned new in /stdc++17 mode and generates a warning otherwise. We would have to check for a warning in the feature test, though - not sure if this is possible.

@msimberg
Copy link
Contributor

@hkaiser yep, adding a feature test seems like a sane thing to do. We'd have to check for a warning with GCC as well, but we can probably add -Werror (or equivalent) to the feature test.

If we don't want to overcomplicate things we could actually leave it up to the user to add -faligned-new. Not sure how much we want to help them there.

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

Successfully merging a pull request may close this issue.

3 participants