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

refactor: structure code for ease of use as module #1

Closed
JohelEGP opened this issue Oct 2, 2022 · 11 comments
Closed

refactor: structure code for ease of use as module #1

JohelEGP opened this issue Oct 2, 2022 · 11 comments
Labels
wontfix This will not be worked on

Comments

@JohelEGP
Copy link
Contributor

JohelEGP commented Oct 2, 2022

I wonder if it's in scope and you're willing to refactor the CMake support for cppfront so that it could be used as a module.

My objective is for this project to be able to be used by https://github.com/JohelEGP/jegp.cmake_modules/#jegpcpp2. As a consequence, it'd be easy to integrate into Compiler Explorer: https://godbolt.org/z/Pq48G6T7f. Even if CE starts supporting the compilation of the generated C++ source, it won't integrate with its CMake project feature due to CMake's lack of support for Cpp2 in CMake.

As for the implementation, the most obvious place seems to be https://github.com/modern-cmake/cppfront/blob/main/cmake/CppfrontHelpers.cmake, which contains the important helpers to make this work. The most obvious challenge is the use of the target cppfront::cppfront. This suggests that an indirection to make it an IMPORTED target instead might viable. That could be done by detecting preexisting support for cppfront, as opposed to compiling it like the main CMakeLists.txt, in a similar fashion to the Cpp2 and cppfront variables used to detect support at https://github.com/JohelEGP/jegp.cmake_modules/#variables.

I'd have loved for this project to replace https://github.com/JohelEGP/jegp.cmake_modules/#jegpcpp2, but it's scope also extends to Cpp2 rather than just cppfront.

@alexreinking
Copy link
Contributor

alexreinking commented Oct 2, 2022

I wonder if it's in scope and you're willing to refactor the CMake support for cppfront so that it could be used as a module.
[...]
The most obvious challenge is the use of the target cppfront::cppfront. This suggests that an indirection to make it an IMPORTED target instead might viable. That could be done by detecting preexisting support for cppfront, as opposed to compiling it like the main CMakeLists.txt

I think this is reasonable... it could be restructured as FindCpp2 (or something that doesn't conflict with the existing cppfront name) which tries to find_package(cppfront) (and any number of fallback strategies) if cppfront::cppfront does not already exist as a target.

@alexreinking alexreinking added the enhancement New feature or request label Oct 2, 2022
@alexreinking
Copy link
Contributor

I'd have loved for this project to replace https://github.com/JohelEGP/jegp.cmake_modules/#jegpcpp2,

jegp_cpp2_target(<target>) seems equivalent to cppfront_enable(TARGETS <targets>...), here.

but it's scope also extends to Cpp2 rather than just cppfront.

I'm not sure there's much of a distinction until competing implementations arise?

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Oct 2, 2022

I'm not sure there's much of a distinction until competing implementations arise?

That's right.

@alexreinking
Copy link
Contributor

Actually, I tried hacking on this a little bit and I'm not sure anymore what problem you're trying to solving, exactly. It seems to me that the cmake-only cppfront-config.cmake package (which takes cppfront-exe as a dependency) already suffices. Can you illustrate (preferably with an example) what the problem is?

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Oct 2, 2022

cppfront-config.cmake is generated. I can't do that in CE.

@alexreinking
Copy link
Contributor

alexreinking commented Oct 2, 2022

Maybe we could talk to Matt about adding this project as an optional "library" on CE? Then the IDE/CMake projects could just find_package(cppfront).

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Oct 2, 2022

That's a great idea!

@alexreinking
Copy link
Contributor

That's a great idea!

I just pinged him on Slack. Happy to make changes here to facilitate CE integration :)

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Oct 2, 2022

Success: https://godbolt.org/z/T5q4b8of5. I replaced my module with CppfrontHelpers.cmake, and in CMakeLists.txt added:

add_library(cppfront_cpp2util INTERFACE)
add_library(cppfront::cpp2util ALIAS cppfront_cpp2util)
target_include_directories(cppfront_cpp2util INTERFACE "${JEGP_CPPFRONT_INCLUDE_DIRECTORIES}")

add_executable(cppfront_cppfront IMPORTED)
add_executable(cppfront::cppfront ALIAS cppfront_cppfront)
set_target_properties(cppfront_cppfront PROPERTIES IMPORTED_LOCATION "${JEGP_CXX2_COMPILER}")

@alexreinking
Copy link
Contributor

alexreinking commented Oct 2, 2022

Great! One little note, this will suffice:

add_library(cppfront::cpp2util INTERFACE IMPORTED)
target_include_directories(cppfront::cpp2util INTERFACE "${JEGP_CPPFRONT_INCLUDE_DIRECTORIES}")

add_executable(cppfront::cppfront IMPORTED)
set_target_properties(cppfront::cppfront PROPERTIES IMPORTED_LOCATION "${JEGP_CXX2_COMPILER}")

IMPORTED targets are allowed to have :: directly in their names 🙂

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Aug 4, 2023

I have switched to using jegp_cpp2_target_sources.
That supports C++ modules: https://cpp2.godbolt.org/z/of6zr8P66.

@alexreinking alexreinking added wontfix This will not be worked on and removed enhancement New feature or request labels Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants