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

Reorganize core library #2530

Open
fweik opened this issue Feb 25, 2019 · 6 comments
Open

Reorganize core library #2530

fweik opened this issue Feb 25, 2019 · 6 comments
Labels

Comments

@fweik
Copy link
Contributor

fweik commented Feb 25, 2019

The code in the core library should be split into src, include and tests folders. This allows for public (those in include) and private (those in src) header files and makes clear what is exported. In cmake one would then only expose the include folder to other targets.

@fweik fweik added the Core label Feb 25, 2019
@fweik fweik added this to the Espresso 4.1 milestone Feb 25, 2019
@KaiSzuttor
Copy link
Member

@KaiSzuttor
Copy link
Member

@fweik
Copy link
Contributor Author

fweik commented Apr 1, 2019

Also #2628.

@jngrad
Copy link
Member

jngrad commented May 10, 2019

Would we have to update .codecov.yml? E.g. in the current state, changing codecov/project/tests to

        paths:
          - src/core/unit_tests/
          - src/pdbparser/unit_tests/
          - src/utils/tests/

and codecov/project/core to

        paths:
          - src/core/
          - !src/core/unit_tests/

(I'm no expert in the codecov YAML syntax, and their docs aren't very helpful, so take these code blocks with a grain of salt.)

@KaiSzuttor KaiSzuttor removed this from the Espresso 4.1 milestone Jul 15, 2019
@jngrad
Copy link
Member

jngrad commented Aug 5, 2022

PR #4541 re-organized src/core and src/script_interface by grouping similar features into submodules:

  • src/core/*.{h,c}pp files are now for the most part declaring or using global variables (snake case filenames), while src/core/*/*.{h,c}pp mostly contain class definitions (Pascal case filenames)
  • src/script_interface/*.{h,c}pp files now all contain the ScriptInterface classes and helper functions, while src/script_interface/*/*.{h,c}pp contains the consumer code that bridges the core classes to the Python classes

I think we should move forward with the Pitchfork layout. In the last 12 months, while refactoring the core and script interface, I found myself in situations where I had to create multiple header files for one source file to comply with separation of concerns or reduce compilation times. The Pitchfork layout automates this by having an include folder for header files of the public interface that is passed to target_include_directories(), and a source folder that contains private header files and source files.

Another benefit is that name collisions becomes impossible. This is important for the script interface, since interface classes sometimes have the same name as the corresponding core classes. This is probably why we have this crude workaround:

target_include_directories(Espresso_script_interface
PUBLIC ${CMAKE_SOURCE_DIR}/src)

The script interface includes the entire project structure, such that one can write #include "core/cells.hpp" instead of #include "cells.hpp", which would be ambiguous if a Cells.hpp class existed in the script interface; filenames in C++ can be case-insensitive and some compilers will throw -Wnonportable-include-path if a name collision occurs between two filenames that only differ in case. It took me a while to figure out why AppleClang was rejecting the Version.hpp filename, and I couldn't replace #include "version.hpp" by #include "config/version.hpp" since that file is auto-generated and lives in the build directory. I ended up renaming the class to CodeVersion.hpp in 1fba163 as a workaround.

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

No branches or pull requests

3 participants