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

Allow splitting C++ commands into separate files #2964

Open
daljit46 opened this issue Aug 15, 2024 · 4 comments
Open

Allow splitting C++ commands into separate files #2964

daljit46 opened this issue Aug 15, 2024 · 4 comments

Comments

@daljit46
Copy link
Member

daljit46 commented Aug 15, 2024

We can do this for Python, but not for C++. This wouldn't merely improve consistency, but potentially reduce compilation times by increasing build parallelism and also allow developer to better reason about their code with enhanced readability.

@Lestropie
Copy link
Member

Technically it's already possible just by moving code from a cmd/*.cpp file into "somewhere appropriate" in src/. The main topics for this Issue are I think:

  1. Where is suitable to put such code.
    If there's a plausible chance of a piece of code being applicable to a hypothetical command other than the one for which the code is written, then it should be possible to determine a suitable filesystem location & namespace in (currently) src/ to make it discoverable. What we don't have a precedent for is where to place code that is absolutely only applicable to one command and one command only.
    I think that our prior design philosophy encourages minimisation of the latter; but that's not to say that it might be applicable in a few scenarios.

  2. Which existing commands warrant refactoring.
    Sorting cmd/*.cpp by line count, > 500 lines is:

    1. mrregister: There's already src/registration/, maybe it just needs more functionality to be moved there.
    2. mrcalc: I'd prefer that much of the code here be generalised such that stack-based arbitrary calculations can be performed on other types of data; eg. Create tsfcalc command to replace tsfmult tsfdivide etc. #339.
    3. tckconvert: Much will be moved into src/dwi/tractography/ by Track files: Implement file format handlers #411.
    4. mrtransform
    5. tckmap: src/dwi/tractography/mapping/ already exists, might just need to move some more functionality there.
    6. mtnormalise: At least the fitting of a 3D polynomial to an image could be functionalised and might be useful elsewhere.
    7. tensor2metric: Possibly src/dwi/tensor.* should become a src/dwi/tensor/* namespace.
    8. fixel2voxel: New classes for fixel dataset handling #2657 could likely move this into core/fixel/.
    9. mrconvert: Can see how splitting functions into different files might help here.
    10. fixelcfestats
    11. mrmath
  3. Whether to allow the core API functions usage() and run() to be split across different files.
    Not sure if this might require changes to compilation?
    Or I suppose could just require that cmd/cmdname.cpp define usage() and run() and each could just be a one-liner that calls eg. MR::cmdname::usage() / MR::cmdname::run().

@daljit46
Copy link
Member Author

Technically it's already possible just by moving code from a cmd/*.cpp file into "somewhere appropriate" in src/.

This is true, but I think the requirement for code to be put in src may be too strong IMO. Suppose for example, that a command contains a function veryLong(), then the developer may want to split this into three separate functions short1(), short2() and short3(). These short functions could possibly reside in three different files (with let's say template instantiations happening in each translation unit). Having those functions abstracted away in src would make very little sense as they're irrelevant to any other commands, but they are purely there for better code structure. An example, where this may be relevant would be mrregister where various types of image registration are performed causing an excessive amount of template instantiations to happen inside the translation unit of mrregister.cpp file.

I think allowing for a folder with a flat structure (no subdirectories) per command may be beneficial. For example, inside mrregister, one may have mrregister.cpp,other1.cpp,other2.cpp,

@Lestropie
Copy link
Member

In the case where code is very much specific to one command only (and is strongly expected to remain so in the future), your proposal is:

cmd/
    mrshortcmd.cpp
    mrnotshortcmd/
        function1.cpp
        function2.cpp
        mrnotshortcmd.cpp

?

That would bear certain resemblance to Python in #2920. I'd be happy with that, with the caveat that it would be nice if CMake would figure it out itself on a per-command basis.


Indeed, maximising similarity with #2920 would be:

cmd/
    mrshortcmd.cpp
    mrmediumcmd/
        function1.cpp
        function2.cpp
        mrmediumcmd.cpp
    mrlongcmd/
        functionA.cpp
        functionB.cpp
        functionC.cpp
        functionD.cpp
        usage.cpp
        run.cpp

I don't know if there's adequate motivation for such. With C++ we tend to use option groups and parsing functions that get buried into header files, which keeps usage() fairly short. Conversely, if those option groups are in fact command-specific, maybe they should be in cmd/mrnotshortcmd/ rather than src/, in which case mrlongcmd/ might have some benefit over mrmediumcmd/. With Python, some commands (population_template in particular) can have very long usage() functions, so having usage() and execute() (couldn't use run() in Python) in separate files I deemed beneficial. Of course you could instead just have a population_template.py that has:

def usage():
    return population_template.usage()
def run():
    return population_template.run()

So the question is two-tiered:

  1. Should C++ permit all three structures?
  2. Should Python only permit two of the three structures (ie. permit mrshortcmd & mrmediumcmd, remove mrlongcmd)?

@daljit46
Copy link
Member Author

daljit46 commented Aug 19, 2024

That would bear certain resemblance to Python in #2920. I'd be happy with that, with the caveat that it would be nice if CMake would figure it out itself on a per-command basis.

Yes, this is what I had in mind. On CMake doing this automatically, I believe it would be fairly easy to do (all you need to do is compile the extra sources as a static/object library). I still think that we should consider getting rid of globbing though (although in practice, I have to admit that so far I haven't encountered any issues yet), but that's a separate discussion I guess.

Should C++ permit all three structures?

In my opinion, that would be unnecessary and overkill.

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

No branches or pull requests

2 participants