Refactor: Include H files in HPP files

[Arc676]

While readability is improved by having declarations in H files and implementations in HPP files, the issue is that this split does not work quite as well with heavily templated code without the proper adjustments. IPPL is currently written such that user code has #include <something.h> and then something.h contains #include "something.hpp". This means that all the HPP files in IPPL are semantically invalid. Their contents only make sense in the context of the files that #include them, and only if the files are referenced in the proper order. From the perspective of a compiler, the HPP files themselves are not valid because they reference symbols that have not been declared.

The solution is to reverse the inclusion direction in the codebase. The HPP files need to #include the H files and user code should #include <something.hpp> instead of the H files. This has minimal impact on user code and ensures that the codebase is semantically valid. This enables the use of static code analysis tools, which can be very beneficial to IPPL development. Language Server Protocols (LSPs) were developed for VSCode, which was used by some team members last year, but have become quite popular (and are an open standard¹) and can also be configured for editors like vim. They provide symbol-aware hints and suggestions (such as determining the types involved in a templated declaration) and you can see compilation errors without leaving the editor. This is currently only possible for CPP files such as those in ALPINE and some of the H files in IPPL.

Enabling static code analysis also makes it possible to employ linter tools such as clang-tidy, which can provide warnings about code that doesn't conform to standardized practices, give suggestions about readability and performance, as well as detect common bugs like constructors that leave data members uninitialized. These kinds of checks can be integrated into code reviews.

Footnotes

1. See clangd ↩

[Arc676]

Hey, hope everything's going alright. Sorry for the radio silence, I had some issues getting IPPL to run and then got distracted by other things. The changes can be browsed in my fork if you want to get an idea of what this would entail. Due to the heavy use of templates, there is limited advantage within the library itself (static analysis only works on the concrete types) with this approach unless C++20 concepts are also introduced (see below). IDEs should be able to better understand the application code (where the templates get instantiated), but I wasn't able to rework the includes in ALPINE since those files have new modifications with which I'm not familiar. I've modified the test files such that they compile with the new inclusion direction, but I couldn't get them to run due to some MPI issue. I could get a basic MPI program running locally but not IPPL.

Adding concepts would make it possible to restrict the kind of data types that can be provided to the library functions. It's impossible for a static analysis tool to verify any function calls on a templated type because there are no restrictions. Although the parameters are called "field", we do not enforce that these types are actually fields. A concept that enforces certain expressions would make it possible to enforce proper template parameters and enable some static analysis, although the extent to which this would improve things remains to be seen.

[aaadelmann]

Hi good to hear from you: looks great, indeed. However, at the moment we do not have free electrons available to finalise what you have begun. We are fighting greater obstacles: #302 #273 to name the most important ones. If you like please go ahead but always synch with the master :) and make sure all test and alpine are running on the CPU. I can offer to test the GPU part. What do you tink?

[Arc676]

I actually never did solve the MPI problem, so I can't run anything locally.

[aaadelmann]

What is exactly your MPI problem?
Would it help to reactivate your PSI account?

[Arc676]

Running the master branch it crashes in IPPL initialization:

Thread 1 "TestCGSolver" received signal SIGILL, Illegal instruction.
0x00007ffff78526b7 in mprotect () from /usr/lib/libasan.so.8
(gdb) bt
#0  0x00007ffff78526b7 in mprotect () from /usr/lib/libasan.so.8
#1  0x00007ffff6fc2219 in mca_base_patcher_patch_apply_binary () from /usr/lib/libopen-pal.so.80
#2  0x00007ffff6fc2310 in ?? () from /usr/lib/libopen-pal.so.80
#3  0x00007ffff6fbcc39 in ?? () from /usr/lib/libopen-pal.so.80
#4  0x00007ffff6f59ef3 in mca_base_framework_components_open () from /usr/lib/libopen-pal.so.80
#5  0x00007ffff6fbc6cf in ?? () from /usr/lib/libopen-pal.so.80
#6  0x00007ffff6f5b190 in mca_base_framework_open () from /usr/lib/libopen-pal.so.80
#7  0x00007ffff6f96072 in opal_common_ucx_mca_register () from /usr/lib/libopen-pal.so.80
#8  0x00007ffff763916e in ?? () from /usr/lib/libmpi.so.40
#9  0x00007ffff6f59ef3 in mca_base_framework_components_open () from /usr/lib/libopen-pal.so.80
#10 0x00007ffff7632171 in ?? () from /usr/lib/libmpi.so.40
#11 0x00007ffff6f5b190 in mca_base_framework_open () from /usr/lib/libopen-pal.so.80
#12 0x00007ffff748becc in ?? () from /usr/lib/libmpi.so.40
#13 0x00007ffff748d54d in ompi_mpi_instance_init () from /usr/lib/libmpi.so.40
#14 0x00007ffff747efd3 in ompi_mpi_init () from /usr/lib/libmpi.so.40
#15 0x00007ffff74be8b3 in PMPI_Init () from /usr/lib/libmpi.so.40
#16 0x0000555555d90182 in ippl::mpi::Environment::Environment (this=0x5020000005b0, argc=@0x7ffff42091b0: 5, argv=@0x7ffff3e09160: 0x7fffffffdce8, 
    comm=@0x7ffff3e09180: 0x5555565c1200 <ompi_mpi_comm_world>) at IPPL_ROOT/src/Communicate/Environment.cpp:14
#17 0x0000555555d80947 in std::make_unique<ippl::mpi::Environment, int&, char**&, ompi_communicator_t*&> () at /usr/include/c++/14.2.1/bits/unique_ptr.h:1076
#18 0x0000555555d79629 in ippl::initialize (argc=@0x7ffff42091b0: 5, argv=0x7fffffffdce8, comm=0x5555565c1200 <ompi_mpi_comm_world>)
    at IPPL_ROOT/src/Ippl.cpp:17
#19 0x0000555555ad3fd6 in main (argc=5, argv=0x7fffffffdce8) at IPPL_ROOT/test/solver/TestCGSolver.cpp:22

A simple MPI test program runs fine, though

#include <mpi.h>
#include <iostream>
int main(int argc, char** argv) {
    MPI_Init(&argc, &argv);
    int rank;
    MPI_Comm_rank(MPI_COMM_WORLD, &rank);
    std::cout << "Hello, World from rank " << rank << std::endl;
    MPI_Finalize();
    return 0;
}

I suppose it wouldn't hurt to have access to Merlin again (I still have my old SSH key for Merlin), although I can't guarantee more than sporadic bursts of activity whenever I have time. I have a busy upcoming week at work.

[aaadelmann]

Your account vinciguerra_a is reactivated!