Remove format_header.sh #1466

MarcelKoch · 2023-11-16T16:00:08Z

This PR removes the format_header.sh in favor of purely relying on clang-format. The .clang-format is updated to allow regrouping of includes. In total, out of the affected 1000 files, only ~200 have non-whitespace changes. Many of those are regarding the ginkgo main header, or previously missing

Include groups (ordered by their prority)

<oneapi/... since we used force-top with that a lot.
<hip/hip_runtime.h> same
'main' include
STL
backend library, i.e. cuda/hip/omp/mpi/...
third party library
Ginkgo main header
Ginkgo headers
Other <...> includes
All "..." includes

This is mostly based on the already used regrouping in format_header.sh.

Notable Changes

The 'main' header of an implementation file now uses "...", instead of <...>. This is required, since clang-format can't deduce the main header otherwise.
force-top is removed and instead handling by using negative priorities in .clang-format. In very few cases this is not sufficient, and there clang-format on/off achieves the same results.
Only 1 empty line between separate groups of includes. It is not possible to adjust that with a clang-format setting.
The <ginkgo/ginkgo.hpp> header is now ordered after standard library includes
Some 'main' headers are not recognized. This concerns especially backend headers, which add stuff to code headers, for example cuda/matrix/batch_struct.hpp will not have core/matrix/batch_struct.hpp as its main header anymore.
Split up implementations, like for the Jacobi preconditioner, don't recognize the 'main' header correctly. One solution could be to use the same splitting approach as for CSR/FBCSR kernels.

Since the format_header.sh script is very complex, I think these changes are a small price to removing it.

upsj · 2023-11-16T16:03:48Z

I like it, if it performs well, this should make IDE integration of our header ordering much smoother

MarcelKoch · 2023-11-16T16:03:51Z

The non-whitespace diff can be obtained with:

git diff -b -B -w -z --ignore-space-at-eol --ignore-blank-lines 195e5bf3..195e5bf3~1

The diff is also attached here.
diff.txt

upsj

LGTM!

core/base/batch_multi_vector_kernels.hpp

core/base/array.cpp

yhmtsai · 2023-11-20T08:14:29Z

<oneapi/.. -> <oneapi/dpl... only the dpl need to be on the top. the other oneapi before dpl may also introduce issues.
clang format does not take care of the "" for local and <> from global, so it may still destroy the regroup
hip_runtime should be in hip group. only a few hip tests request it on top which can be handled by clang-format off
9 can be merged into 6?
It changes a lot of the format rules in the documentation
I would say this pr changes the format style not just remove format_header

yhmtsai

the regroup does not take care the header. for example cb_gmres example

MarcelKoch · 2023-11-20T08:25:28Z

The main goal of the PR is to remove format_header.sh. The formatting changes are a by product of that.

9 can be merged into 6?

I don't follow, can you elaborate?

the regroup does not take care the header. for example cb_gmres example

Do you mean the wrong comment on the main ginkgo header file?

MarcelKoch · 2023-11-20T08:30:58Z

clang format does not take care of the "" for local and <> from global, so it may still destroy the regroup

I think it does. Any <> is priority 1-4, the <ginkgo/ headers are priority 5-6, and everything else, which includes "" is priority 7.

pratikvn

LGTM!

upsj

LGTM!

yhmtsai · 2024-06-25T14:25:32Z

CONTRIBUTING.md

 In general, all include statements should be present on the top of the file,
-ordered in the following groups, with two blank lines between each group:
+ordered in the following groups, with *one* blank lines between each group:



the following is slightly different from the clang_format group rule such as the ginkgo main header is in another group

yhmtsai · 2024-06-25T14:44:26Z

cuda/test/base/array.cpp

 #include <gtest/gtest.h>

-
+#include <ginkgo/core/base/array.hpp>


Suggested change

#include <ginkgo/core/base/array.hpp>

#include "ginkgo/core/base/array.hpp"

such that it can be recognized as a main header

This is not the main header. The conversation about main headers only makes sense for files which implement functions defined in a header file. This is not the case for the test files, so there is no main header here.
I think we had a different interpretation of main header previously, which was based on ~vibes~. Now we just follow clangs definition.
So all the comments on the test files are unnecessary. Sorry for not making that clearer before.

with this definition, the opposite part needs to change.
cuda/test/matrix/coo_kernels.cpp uses core/matrix/coo_kernels.hpp as the main header because they are in the mainHeaderSource

do you mean cuda/matrix/coo_kernels.cpp? Because there is no cuda/test/matrix/coo_kernels.cpp.

sorry, I just randomly pick something might have issue.
something like this: cuda/test/solver/lower_trs_kernels.cu uses core/solver/lower_trs_kernels.hpp

I see. This poses an issue, because there doesn't seem to be a way to prevent clang from detecting core/solver/lower_trs_kernels.hpp as main header. I think the simplest choice is to consider these headers as the main header again.

Side note: this particular header is unnecessary. The test doesn't call any kernel directly.

Or we can just ignore it and live with these (small) inconsistencies. That means there will be some tests which have a main header and those that don't.

yhmtsai · 2024-06-25T14:44:48Z

cuda/test/base/cuda_executor.cu

 #include <ginkgo/core/base/exception.hpp>
 #include <ginkgo/core/base/exception_helpers.hpp>
+#include <ginkgo/core/base/executor.hpp>


main header

yhmtsai · 2024-06-25T14:45:10Z

cuda/test/base/exception_helpers.cu

 #include <gtest/gtest.h>

+#include <ginkgo/core/base/exception_helpers.hpp>


main header

yhmtsai · 2024-06-25T14:45:21Z

cuda/test/base/index_set.cpp

 #include <ginkgo/core/base/exception_helpers.hpp>
 #include <ginkgo/core/base/executor.hpp>
+#include <ginkgo/core/base/index_set.hpp>


main header

yhmtsai · 2024-06-25T16:18:33Z

reference/test/stop/iteration.cpp

 #include <gtest/gtest.h>

+#include <ginkgo/core/stop/iteration.hpp>


main header

yhmtsai · 2024-06-25T16:18:41Z

reference/test/stop/residual_norm_kernels.cpp

 #include <ginkgo/core/base/math.hpp>
-
+#include <ginkgo/core/stop/residual_norm.hpp>


main header

yhmtsai · 2024-06-25T16:18:49Z

reference/test/stop/time.cpp

 #include <chrono>
 #include <thread>
+
+#include <ginkgo/core/stop/time.hpp>


main header

yhmtsai · 2024-06-25T16:19:14Z

test/base/timer.cpp

 #include <gtest/gtest.h>

+#include <ginkgo/core/base/timer.hpp>


main header

yhmtsai · 2024-06-25T16:19:58Z

test/matrix/csr_kernels2.cpp

 #include <ginkgo/core/base/exception.hpp>
 #include <ginkgo/core/base/executor.hpp>
 #include <ginkgo/core/matrix/coo.hpp>
+#include <ginkgo/core/matrix/csr.hpp>


main header

yhmtsai

summary of my comments:

oneapi except for dpl should be in the same group as cu/hip...
sde_lib is also from papi. and maybe we can put the priority 4 and 5 together?
some main headers are missed or wrong due to no "".
because dpl in priority -2, we do not need the clang-format on/off to force it. It may apply to runtime but I am not sure the priority 2 is enough for solving the compiling issue.

yhmtsai

I ignore main header inconsistentence for now.
the onedpl with disabling the clang-format is not changed and some wrong usage of header ""/<> is not resolved yet.

yhmtsai · 2024-06-27T12:03:13Z

core/base/batch_multi_vector_kernels.hpp

@@ -6,13 +6,11 @@
 #define GKO_CORE_BASE_BATCH_MULTI_VECTOR_KERNELS_HPP_


-#include <ginkgo/core/base/batch_multi_vector.hpp>
-
+#include "ginkgo/core/base/batch_multi_vector.hpp"


I thought the hpp is out of the maing header check?

you're right, I will update this (and the others) accordingly

yhmtsai · 2024-06-27T12:04:10Z

core/base/device_matrix_data_kernels.hpp

@@ -6,16 +6,13 @@
 #define GKO_CORE_BASE_DEVICE_MATRIX_DATA_KERNELS_HPP_


-#include <ginkgo/core/base/device_matrix_data.hpp>
-
+#include "ginkgo/core/base/device_matrix_data.hpp"


same question

yhmtsai · 2024-06-27T12:04:59Z

core/base/utils.hpp

 #include <ginkgo/core/base/polymorphic_object.hpp>
 #include <ginkgo/core/base/types.hpp>
 #include <ginkgo/core/matrix/csr.hpp>

+#include "ginkgo/core/base/utils.hpp"


because no main header, use <> not ""

yhmtsai · 2024-06-27T12:06:09Z

core/distributed/index_map_kernels.hpp

@@ -6,14 +6,12 @@
 #define GKO_CORE_DISTRIBUTED_INDEX_MAP_KERNELS_HPP_


-#include <ginkgo/core/distributed/index_map.hpp>
-
+#include "ginkgo/core/distributed/index_map.hpp"


same question for hpp

yhmtsai · 2024-06-27T12:08:58Z

core/reorder/mc64.hpp

 #include "core/components/addressable_pq.hpp"
+#include "ginkgo/core/reorder/mc64.hpp"


should use <>

TODO: remove or revert this commit

changes from: `./dev_tools/scripts/change-main-include.py $(git ls-files)` This is done based on the heuristic that the first include is the main include, except in some cases. Here are the cases where the heuristic can't determine either way: benchmark/utils/mpi_timer.cpp benchmark/utils/tuning_variables.cpp core/base/mpi.cpp core/base/segmented_array.cpp core/base/version.cpp core/config/multigrid_config.cpp core/distributed/index_map.cpp core/distributed/partition.cpp core/log/logger.cpp core/preconditioner/batch_jacobi.cpp core/stop/combined.cpp core/stop/criterion.cpp core/stop/iteration.cpp core/stop/time.cpp cuda/base/version.cpp devices/cuda/executor.cpp devices/hip/executor.cpp dpcpp/base/version.dp.cpp hip/base/version.hip.cpp omp/base/version.cpp reference/base/version.cpp

Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>

This reverts commit a9880c2.

MarcelKoch · 2024-06-28T07:56:06Z

I've rebased it after the changes from #1616. It went pretty smoothly. I don't think there are additional changes that were not reviewed before, so I will go a head and merge it when the tests pass.

MarcelKoch self-assigned this Nov 16, 2023

Base automatically changed from use-fixed-clang-format to develop November 16, 2023 18:02

MarcelKoch force-pushed the remove-format_header branch from a2f77d4 to b331a11 Compare November 16, 2023 18:21

upsj approved these changes Nov 16, 2023

View reviewed changes

pratikvn reviewed Nov 17, 2023

View reviewed changes

core/base/batch_multi_vector_kernels.hpp Show resolved Hide resolved

core/base/array.cpp Show resolved Hide resolved

MarcelKoch force-pushed the remove-format_header branch from b331a11 to 7af592f Compare November 17, 2023 08:36

MarcelKoch added the 1:ST:do-not-merge Please do not merge PR this yet. label Nov 17, 2023

MarcelKoch force-pushed the remove-format_header branch from 7af592f to 2a6d96f Compare November 20, 2023 08:07

yhmtsai reviewed Nov 20, 2023

View reviewed changes

MarcelKoch force-pushed the remove-format_header branch from 2a6d96f to f8b038c Compare November 20, 2023 13:56

MarcelKoch force-pushed the remove-format_header branch 2 times, most recently from 70bb6df to 0d2e147 Compare June 14, 2024 12:51

MarcelKoch requested review from upsj, pratikvn and yhmtsai June 17, 2024 12:15

pratikvn approved these changes Jun 17, 2024

View reviewed changes

MarcelKoch force-pushed the remove-format_header branch from 0d2e147 to ba448c9 Compare June 17, 2024 13:37

upsj approved these changes Jun 17, 2024

View reviewed changes

MarcelKoch added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:do-not-merge Please do not merge PR this yet. labels Jun 18, 2024

yhmtsai requested changes Jun 25, 2024

View reviewed changes

yhmtsai approved these changes Jun 27, 2024

View reviewed changes

MarcelKoch and others added 13 commits June 28, 2024 09:06

adds script to change main include to use "" instead of <>

a9880c2

TODO: remove or revert this commit

manually changed main header

c651aff

replace force-top with clang-format on/off

35b90f3

remove format_header.sh

31d2ef1

remove formatting from convert_source.sh

e32e0e7

use clang-format to format includes

bf75f13

Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>

remove force-top where made unnecessary

3990596

fixup! use clang-format to format includes

7632513

automatically update header ordering with clang-format

c3485ea

update contributing documentation

e6770a7

[test] fix whitespace bug in profiler_hook test

91ccf49

Revert "adds script to change main include to use "" instead of <>"

e4d744e

This reverts commit a9880c2.

MarcelKoch force-pushed the remove-format_header branch from c545d97 to e4d744e Compare June 28, 2024 07:54

MarcelKoch merged commit 5363f07 into develop Jun 28, 2024
10 of 14 checks passed

MarcelKoch deleted the remove-format_header branch June 28, 2024 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove format_header.sh #1466

Remove format_header.sh #1466

MarcelKoch commented Nov 16, 2023

upsj commented Nov 16, 2023

MarcelKoch commented Nov 16, 2023

upsj left a comment

yhmtsai commented Nov 20, 2023

yhmtsai left a comment

MarcelKoch commented Nov 20, 2023

MarcelKoch commented Nov 20, 2023

pratikvn left a comment

upsj left a comment

yhmtsai Jun 25, 2024

yhmtsai Jun 25, 2024

MarcelKoch Jun 26, 2024

yhmtsai Jun 26, 2024

MarcelKoch Jun 26, 2024

yhmtsai Jun 26, 2024

MarcelKoch Jun 26, 2024

MarcelKoch Jun 26, 2024

yhmtsai Jun 25, 2024

yhmtsai Jun 25, 2024

yhmtsai Jun 25, 2024

yhmtsai Jun 25, 2024

yhmtsai Jun 25, 2024

yhmtsai Jun 25, 2024

yhmtsai Jun 25, 2024

yhmtsai Jun 25, 2024

yhmtsai left a comment

yhmtsai left a comment

yhmtsai Jun 27, 2024

MarcelKoch Jun 28, 2024

yhmtsai Jun 27, 2024

yhmtsai Jun 27, 2024

yhmtsai Jun 27, 2024

yhmtsai Jun 27, 2024

MarcelKoch commented Jun 28, 2024

		#include <gtest/gtest.h>


		#include <ginkgo/core/base/array.hpp>

	#include <ginkgo/core/base/array.hpp>
	#include "ginkgo/core/base/array.hpp"

		#include <gtest/gtest.h>

		#include <ginkgo/core/base/exception_helpers.hpp>

		#include <gtest/gtest.h>

		#include <ginkgo/core/stop/iteration.hpp>

		#include <ginkgo/core/base/math.hpp>

		#include <ginkgo/core/stop/residual_norm.hpp>

		#include <gtest/gtest.h>

		#include <ginkgo/core/base/timer.hpp>

		#include "core/components/addressable_pq.hpp"
		#include "ginkgo/core/reorder/mc64.hpp"

Remove format_header.sh #1466

Remove format_header.sh #1466

Conversation

MarcelKoch commented Nov 16, 2023

Include groups (ordered by their prority)

Notable Changes

upsj commented Nov 16, 2023

MarcelKoch commented Nov 16, 2023

upsj left a comment

Choose a reason for hiding this comment

yhmtsai commented Nov 20, 2023

yhmtsai left a comment

Choose a reason for hiding this comment

MarcelKoch commented Nov 20, 2023

MarcelKoch commented Nov 20, 2023

pratikvn left a comment

Choose a reason for hiding this comment

upsj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhmtsai left a comment

Choose a reason for hiding this comment

yhmtsai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcelKoch commented Jun 28, 2024