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] Move parse_options.cpp and cmdline.cpp under src/cli_utils/. #7784

Closed
wants to merge 1 commit into from

Conversation

NlightNFotis
Copy link
Contributor

We want to start breaking down the src/util/ folder in a manner that makes its constituents more organised in a logical hierarchy and also to allow us to set up better boundaries between different modules (and allow the build system to evolve in a way that allows more selective inclusion of different components in a library built).


There's still work to be done here (we expect the Make builds to fail as we haven't ported the make built to the new hierarchy yet), but we want to make sure that the community aligns with our aims and process here before we commit to more (tedious) work.

We want to start breaking down the `src/util/` folder in a manner
that makes its constituents more organised in a logical hierarchy
and also to allow us to set up better boundaries between different
modules (and allow the build system to evolve in a way that allows
more selective inclusion of different components in a library built).
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.58%. Comparing base (da48fba) to head (78e590b).
Report is 1273 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #7784   +/-   ##
========================================
  Coverage    78.58%   78.58%           
========================================
  Files         1693     1693           
  Lines       193381   193381           
========================================
  Hits        151962   151962           
  Misses       41419    41419           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tautschnig
Copy link
Collaborator

I'm afraid that we'll end up with a very large number of folders right under src/ if we go down this route. You might want to first explore introducing sub-folders insrc/util/.

@tautschnig
Copy link
Collaborator

As a further note: can we take a first small step and move log_version_and_architecture to config.cpp so as to remove the dependency on config.h from parse_options.cpp? I suggest we eventually separate out everything that doesn't (transitively) depend on irep.h (and should try to make that set of files as large as possible).

@thomasspriggs
Copy link
Contributor

I'm afraid that we'll end up with a very large number of folders right under src/ if we go down this route. You might want to first explore introducing sub-folders insrc/util/.

@esteffin Can you explain the build-system related concerns you had about that? I guess the way /src/util/CMakeLists.txt is currently written using GLOB_RECURSE means it would automatically include the .cpp files in a /src/util/cli/ directory. But could we just switch to using just GLOB in order to retain management of the two directories as two separate modules?

@esteffin
Copy link
Contributor

@tautschnig
The aim of this PR was to start splitting the util library as it is growing feeling that it has to be split in separate units such as irept and derivatives, global components such as optionst and components for executable creation such as parse_option_baset (that was moved here).

However the extent of the work mean that the split will happen in a number of separate PR with the outcome of having an unstabilized util for a while.

To avoid an organic split, that can affect the final outcome, we decided to open an RFC (soon to be created) where the fate of util will be detailed and discussed both in term of folder structure and in term of build system and artifact produced.

I will leave this PR open and reference it in the RFC, but it will be unlikely progressing until a more consensual decision on the structure of util will be taken.

@NlightNFotis
Copy link
Contributor Author

Similar to #8110 , I don't see having the time to see this through any time soon, therefore I will close this PR in an attempt to keep the repository tidy.

If anyone wants to work on this one and get it merged, feel free to re-open this PR, or to use the branch as a basis for a separate PR. Thanks for your understanding.

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.

4 participants