format the header of files

[yhmtsai]

This PR sorts the ordering of headers, use correct "header" or <header>, and correct the definition of header.

1. Add hip/hip_runtime.h into hip/base/config.hip.hpp as its main include to ensure it is in front of hip/device_functions.h mentioned in Some header files are not self-contained #412
2. use IncludeBlocks: Regroup (need clang-format 6) to sort the header except the main include
3. add config (dev_tools/scripts/config) to set the rules how to find the corresponding main include
4. if there are something like comment of #if block before #include, the script will give alarm "sorting is maybe not correct"
5. Also alarm if there are multiple matches the main include.

Also mention this in #389 , but remove many if-else and unclear regex (which is almost handled by config)

add target format_header and format_header_all in CMake
format_header only process the changed files of branch
format_header_all process all code files.
They ignore build, examples, third_party, ginkgo.hpp

add // force-top: on and // force-top: off to keep some headers in the top (also before the main header).
I change the warning condition. If there are comments or #if block before codes, it gives warning.
The following is warning

Warning benchmark/utils/formats.hpp: sorting maybe is not correct
Warning core/base/extended_float.hpp: sorting maybe is not correct
Warning core/device_hooks/common_kernels.inc.cpp: sorting maybe is not correct
Warning include/ginkgo/core/base/abstract_factory.hpp: sorting maybe is not correct
Warning include/ginkgo/core/base/name_demangling.hpp: sorting maybe is not correct
Warning include/ginkgo/core/base/types.hpp: sorting maybe is not correct
Warning include/ginkgo/core/base/utils.hpp: sorting maybe is not correct
Warning reference/test/stop/combined.cpp: sorting maybe is not correct
Warning reference/test/stop/time.cpp: sorting maybe is not correct

[yhmtsai]

6 parts for reviews

• config/scripts review
• core changes review
• cuda changes review
• hip changes review
• include/ginkgo/core changes review
• omp changes review
• referece changes review

Also, the following is the alarm of script

(base) mike:ginkgo$ find core -type f -not -name "*.txt" -exec  dev_tools/scripts/format_header.sh {} \;
(base) mike:ginkgo$ find cuda -type f -not -name "*.txt" -exec  dev_tools/scripts/format_header.sh {} \;
(base) mike:ginkgo$ find hip -type f -not -name "*.txt" -exec  dev_tools/scripts/format_header.sh {} \;
(base) mike:ginkgo$ find include/ginkgo/core -type f -not -name "*.txt" -exec  dev_tools/scripts/format_header.sh {} \;
ALARM include/ginkgo/core/base/name_demangling.hpp may not be sorted correctly
(base) mike:ginkgo$ find omp -type f -not -name "*.txt" -exec  dev_tools/scripts/format_header.sh {} \;
(base) mike:ginkgo$ find reference -type f -not -name "*.txt" -exec  dev_tools/scripts/format_header.sh {} \;

In name_demangling.hpp, it contains #if block before other #include

However, if #if block is in the end, it will not give an alarm like reference/test/stop/combined.cpp and reference/test/stop/time.cpp

[upsj]

LGTM in general, I noticed some places where the script seems to contradict our guidelines, probably due to inconsistent file naming?

[upsj]

LGTM (modulo compilation issues)

[yhmtsai]

@upsj How do you think about combining prefix_sum.hip.hpp and prefix_sum.hip.cpp into prefix_sum_kernels.hip.cpp?

1. the interface is already defined in core/components/prefix_sum.hpp
2. It will be the same as the others like matrix and solver (with kernels suffix and the main header in core)

[upsj]

@upsj How do you think about combining prefix_sum.hip.hpp and prefix_sum.hip.cpp into prefix_sum_kernels.hip.cpp?

1. the interface is already defined in core/components/prefix_sum.hpp

2. It will be the same as the others like matrix and solver (with kernels suffix and the main header in core)

My answer to that would probably be "yes and/or unsure": At the moment, prefix_sum.hip.hpp only contains things that are used in prefix_sum.hip.cpp. For ParILUT however, I need a "wide" prefix sum kernel that is quite specific to the selection implementation and uses device functions I also put into prefix_sum.hip.hpp (or more specifically, prefix_sum.hpp.inc). So what do you think makes more sense:

• Leave header and source separated, but rename the source to _kernels?
• Combine header and source and later on also add the wide_prefix_sum to this file, even though it is only used by ParILUT?

[pratikvn]

While this header overhaul is being done, maybe it is a good idea to also make it up to date with the recommendations of include-what-you-use from the CI ?

This would definitely take more time and be annoying, so if you want to leave that to another PR in the future that is also fine.

[thoasm]

First part of my review.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.6% Duplication

[yhmtsai]

@upsj i go for the option 1 but change others to _kernels in the new commit. ( Maybe use _impl or something else as the keywords said the main header is in the same folder)
In components, if the filename contain _kernels, its main header is in the same folder.
If not, its main header is the header with the same name in core, which is like the behaviour of other files in matrix or solver.

[thoasm]

LGTM! Your script caught some interesting (minor) errors that we would have not caught otherwise.
Some minor comments, but nothing serious.

[codecov]

Codecov Report

Merging #447 into develop will decrease coverage by 0.1%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #447      +/-   ##
===========================================
- Coverage    88.86%   88.75%   -0.11%     
===========================================
  Files          256      256              
  Lines        16484    16491       +7     
===========================================
- Hits         14648    14637      -11     
- Misses        1836     1854      +18

Impacted Files	Coverage Δ
core/test/base/range.cpp	100% <ø> (ø)	⬆️
omp/test/preconditioner/jacobi_kernels.cpp	100% <ø> (ø)	⬆️
omp/test/matrix/ell_kernels.cpp	100% <ø> (ø)	⬆️
include/ginkgo/core/base/array.hpp	96.42% <ø> (ø)	⬆️
include/ginkgo/core/solver/gmres.hpp	100% <ø> (ø)	⬆️
reference/test/solver/lower_trs_kernels.cpp	100% <ø> (ø)	⬆️
core/test/stop/time.cpp	100% <ø> (ø)	⬆️
reference/test/stop/criterion_kernels.cpp	100% <ø> (ø)	⬆️
core/stop/residual_norm_reduction.cpp	81.25% <ø> (ø)	⬆️
include/ginkgo/core/log/record.hpp	91.48% <ø> (ø)	⬆️
... and 80 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9435b45...7b45b6a. Read the comment docs.

[yhmtsai]

It is updated.

add target format_header and format_header_all in CMake
format_header only process the changed files of branch
format_header process all code files.
They ignore build, examples, third_party, ginkgo.hpp

add // force-top: on and // force-top: off to keep some headers in the top (also before the main header).
I change the warning condition. If there are comments or #if block before codes, it gives warning.
The following is warning

Warning benchmark/utils/formats.hpp: sorting maybe is not correct
Warning core/base/extended_float.hpp: sorting maybe is not correct
Warning core/device_hooks/common_kernels.inc.cpp: sorting maybe is not correct
Warning include/ginkgo/core/base/abstract_factory.hpp: sorting maybe is not correct
Warning include/ginkgo/core/base/name_demangling.hpp: sorting maybe is not correct
Warning include/ginkgo/core/base/types.hpp: sorting maybe is not correct
Warning include/ginkgo/core/base/utils.hpp: sorting maybe is not correct
Warning reference/test/stop/combined.cpp: sorting maybe is not correct
Warning reference/test/stop/time.cpp: sorting maybe is not correct

[pratikvn]

Just a question, is this a blocking change ? I see that if GINKGO_DEVEL_TOOLS is on, then the format_header script is called. Currently, the user needs to call make format_header_all to format the files. Do we want to do something similar as for make format ? Or is the way it is now, enough ?

[pratikvn]

Great work! Only minor typos. LGTM!

[yhmtsai]

@pratikvn No, it is not a blocking changes now.
Currently, I would prefer to leave it as an option.
we can try it for a while. If everything is expected, we can also add it like git-cmake-format.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

No Coverage information
0.3% Duplication