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

Remove format_header.sh #1466

Merged
merged 13 commits into from
Jun 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
25 changes: 24 additions & 1 deletion .clang-format
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,30 @@ ForEachMacros:
- foreach
- Q_FOREACH
- BOOST_FOREACH
IncludeIsMainRegex: '([-_](test|unittest))?$'
IncludeBlocks: Regroup
IncludeCategories:
- Regex: '^<oneapi/dpl.*' # needs to be on top
Priority: -2
- Regex: '<[^.]+>' # standard library
Priority: 1
- Regex: '(^<(hip/hip_runtime|cuda(_runtime)?)\.h)|common/cuda_hip/base/runtime\.hpp$'
Priority: 2
SortPriority: 2
- Regex: '^<(omp|cu|hip|oneapi|thrust|CL/|cooperative|mpi|nvToolsExt|Kokkos).*'
Priority: 2
SortPriority: 3
- Regex: '^<(nlohmann|gflags|gtest|sde_lib|papi).*'
Priority: 4
- Regex: '<ginkgo/ginkgo.hpp>'
Priority: 6
- Regex: '^<ginkgo/.*'
Priority: 7
- Regex: '^<.*' # other library includes
Priority: 5
- Regex: '^.*'
Priority: 8
IncludeIsMainRegex: '(_(stub|kernels|kernels2|test))?$'
IncludeIsMainSourceRegex: '\.cu$|_kernels\.hpp$|\.dp\.cpp$'
IndentCaseLabels: false
IndentWidth: 4
IndentWrappedFunctionNames: false
Expand Down
2 changes: 0 additions & 2 deletions .github/bot-pr-base.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
source .github/bot-base.sh

EXTENSION_REGEX='\.(cuh?|hpp|hpp\.inc?|cpp)$'
FORMAT_HEADER_REGEX='^(benchmark|core|cuda|hip|include/ginkgo/core|omp|reference|dpcpp|common/unified|test)/'
FORMAT_REGEX='^(common|examples)/'
CLANG_FORMAT=clang-format-14

echo -n "Collecting information on triggering PR"
Expand Down
5 changes: 1 addition & 4 deletions .github/bot-pr-format-base.sh
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ git config user.name "ginkgo-bot"

# save scripts from develop
cp .clang-format .pre-commit-config.yaml /tmp
pushd dev_tools/scripts || exit 1
cp format_header.sh update_ginkgo_header.sh /tmp
popd || exit 1
cp dev_tools/scripts/update_ginkgo_header.sh /tmp

# checkout current PR head
LOCAL_BRANCH=format-tmp-$HEAD_BRANCH
Expand All @@ -25,7 +23,6 @@ git checkout -b $LOCAL_BRANCH fork/$HEAD_BRANCH
# restore files from develop
cp /tmp/.clang-format .
cp /tmp/.pre-commit-config.yaml .
cp /tmp/format_header.sh dev_tools/scripts/
cp /tmp/update_ginkgo_header.sh dev_tools/scripts/

# make base pre-commit config available
Expand Down
16 changes: 2 additions & 14 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ repos:
exclude: |
(?x)^(
third_party/SuiteSparse/AMD/.*|
third_party/identify_stream_usage/.*
third_party/identify_stream_usage/.*|
include/ginkgo/ginkgo.hpp
)
- repo: local
hooks:
Expand All @@ -25,19 +26,6 @@ repos:
examples/external-lib-interfacing/external-lib-interfacing.cpp|
core/base/workspace_aliases.hpp
)$
- id: format-headers
name: format headers
entry: env CLANG_FORMAT=dev_tools/scripts/clang-format.sh dev_tools/scripts/format_header.sh
require_serial: true
language: system
types_or: [c, c++, cuda]
exclude: |
(?x)^(
third_party/SuiteSparse/AMD/.*|
third_party/identify_stream_usage/.*|
include/ginkgo/ginkgo.hpp|
core/base/workspace_aliases.hpp
)$
- id: update-ginkgo-header
name: update ginkgo header
entry: dev_tools/scripts/update_ginkgo_header.sh
Expand Down
20 changes: 0 additions & 20 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -367,26 +367,6 @@ if(NOT "${BASH}" STREQUAL "BASH-NOTFOUND" AND GINKGO_DEVEL_TOOLS)
add_custom_target(generate_ginkgo_header ALL
COMMAND ${Ginkgo_SOURCE_DIR}/dev_tools/scripts/update_ginkgo_header.sh
WORKING_DIRECTORY ${Ginkgo_SOURCE_DIR})
find_program(GIT git)
if(NOT "${GIT}" STREQUAL "GIT-NOTFOUND")
add_custom_target(format_header
COMMAND echo "format header on the modified code files except build, examples, third_party, accessor/, dev_tools, ginkgo.hpp"
COMMAND bash -c "git diff --name-only origin/develop...HEAD | \
grep -Ev 'build|examples|third_party|accessor/|dev_tools|ginkgo.hpp' | \
grep -E '(\.hip)?\.(cu|hpp|cuh|cpp)$' | \
xargs -r -n1 ${Ginkgo_SOURCE_DIR}/dev_tools/scripts/format_header.sh"
WORKING_DIRECTORY ${Ginkgo_SOURCE_DIR}
VERBATIM)
endif()
unset(GIT CACHE)
add_custom_target(format_header_all
COMMAND echo "format header on all code files except build, examples, third_party, accessor/, dev_tools, ginkgo.hpp"
COMMAND bash -c "find * -type f | \
grep -Ev 'build|examples|third_party|accessor/|dev_tools|ginkgo.hpp' | \
grep -E '(\.hip)?\.(cu|hpp|cuh|cpp)$' | \
xargs -r -n1 ${Ginkgo_SOURCE_DIR}/dev_tools/scripts/format_header.sh"
WORKING_DIRECTORY ${Ginkgo_SOURCE_DIR}
VERBATIM)
endif()
unset(BASH CACHE)

Expand Down
62 changes: 22 additions & 40 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -176,12 +176,12 @@ improvements from code reviews.

### Automatic code formatting

Ginkgo uses [ClangFormat](https://clang.llvm.org/docs/ClangFormat.html)
(executable is usually named `clang-format`) and a custom `.clang-format`
configuration file (mostly based on ClangFormat's _Google_ style) to
automatically format your code. __Make sure you have ClangFormat set up and
running properly__ ( you should be able to run `make format` from Ginkgo's build
directory) before committing anything that will end up in a pull request against
Ginkgo uses [pre-commit](https://pre-commit.com/) to automatically apply
code formatting when committing changes to git. What formatting is applied
is managed through [ClangFormat](https://clang.llvm.org/docs/ClangFormat.html)
with a custom `.clang-format` configuration file (mostly based on ClangFormat's
_Google_ style). __Make sure you have pre-commit set up and running properly__
before committing anything that will end up in a pull request against
`ginkgo-project/ginkgo` repository. In addition, you should __never__ modify the
`.clang-format` configuration file shipped with Ginkgo. E.g. if ClangFormat has
trouble reading this file on your system, you should install a newer version of
Expand Down Expand Up @@ -339,64 +339,53 @@ Thus, contributors should be aware of the following rules for blank lines:

### Include statement grouping

The concrete ordering will be done by `clang-format`.
Here are the rules that `clang-format` will follow.
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:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

1. Related header file (e.g. `core/foo/bar.hpp` included in `core/foo/bar.cpp`,
1. Main header file (e.g. `core/foo/bar.hpp` included in `core/foo/bar.cpp`,
or in the unit test`core/test/foo/bar.cpp`)
2. Standard library headers (e.g. `vector`)
3. Executor specific library headers (e.g. `omp.h`)
4. System third-party library headers (e.g. `papi.h`)
5. Local third-party library headers
6. Public Ginkgo headers
7. Private Ginkgo headers
5. Public Ginkgo headers
6. Local headers

_Example_: A file `core/base/my_file.cpp` might have an include list like this:

```c++
#include <ginkgo/core/base/my_file.hpp>

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

#include <algorithm>
#include <vector>
#include <tuple>


#include <omp.h>


#include <papi.h>


#include "third_party/blas/cblas.hpp"
#include "third_party/lapack/lapack.hpp"

#include <third_party/blas/cblas.hpp>
#include <third_party/lapack/lapack.hpp>

#include <ginkgo/core/base/executor.hpp>
#include <ginkgo/core/base/lin_op.hpp>
#include <ginkgo/core/base/types.hpp>


#include "core/base/my_file_kernels.hpp"
```

#### Main header

This section presents general rules used to define the main header attributed to
the file. In the previous example, this would be ` #include
<ginkgo/core/base/my_file.hpp>`.
This section presents the handling of the main header attributed to a file.
For a given file, the main header is the header that contains the declarations of the
functions, classes, etc., which are implemented in this file.
In the previous example, this would be ` #include "ginkgo/core/base/my_file.hpp"`.
The `clang-format` tool figures out the main header. The only intervention form
a contributor is to *always* include the main header using `"..."`.

General rules:
1. Some fixed main header.
2. components:
- with `_kernel` suffix looks for the header in the same folder.
- without `_kernel` suffix looks for the header in `core`.
3. `test/utils`: looks for the header in `core`
4. `core`: looks for the header in `ginkgo`
5. `test` or `base`: looks for the header in `ginkgo/core`
6. others: looks for the header in `core`
Please note that this only applies to implementation files, so files ending in `.cpp` or `.cu`.

_Note_: Please see the detail in the `dev_tools/scripts/config`.

#### Some general comments.

Expand All @@ -405,13 +394,6 @@ _Note_: Please see the detail in the `dev_tools/scripts/config`.
When compiling with `GINKGO_CHECK_CIRCULAR_DEPS` enabled, this property is explicitly checked.
3. The recommendations of the `iwyu` (Include what you use) tool can be used to make sure that the headers are self-sufficient and that the compiled files ( `.cu`, `.cpp`, `.hip.cpp` ) include only what they use. A [CI pipeline](https://gitlab.com/ginkgo-project/ginkgo-public-ci/-/jobs/584358356) is available that runs with the `iwyu` tool. Please be aware that this tool can be incorrect in some cases.

#### Automatic header arrangement

1. `dev_tools/script/format_header.sh` will take care of the group/sorting of
headers according to this guideline.
2. `make format_header` arranges the header of the modified files in the branch.
3. `make format_header_all` arranges the header of all files.


### Other Code Formatting not handled by ClangFormat

Expand Down
1 change: 0 additions & 1 deletion accessor/accessor_helper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#include <type_traits>
#include <utility>


#include "index_span.hpp"
#include "utils.hpp"

Expand Down
1 change: 0 additions & 1 deletion accessor/block_col_major.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include <array>
#include <cinttypes>


#include "accessor_helper.hpp"
#include "range.hpp"
#include "utils.hpp"
Expand Down
2 changes: 0 additions & 2 deletions accessor/cuda_helper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@

#include <type_traits>


#include <thrust/complex.h>


#include "block_col_major.hpp"
#include "reduced_row_major.hpp"
#include "row_major.hpp"
Expand Down
2 changes: 0 additions & 2 deletions accessor/hip_helper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@

#include <type_traits>


#include <thrust/complex.h>


#include "block_col_major.hpp"
#include "reduced_row_major.hpp"
#include "row_major.hpp"
Expand Down
1 change: 0 additions & 1 deletion accessor/math.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include <type_traits>


#include "utils.hpp"


Expand Down
1 change: 0 additions & 1 deletion accessor/range.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

#include <utility>


#include "utils.hpp"


Expand Down
1 change: 0 additions & 1 deletion accessor/reduced_row_major.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include <type_traits>
#include <utility>


#include "accessor_helper.hpp"
#include "index_span.hpp"
#include "range.hpp"
Expand Down
1 change: 0 additions & 1 deletion accessor/reduced_row_major_reference.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include <cmath>
#include <type_traits>


#include "math.hpp"
#include "reference_helper.hpp"
#include "utils.hpp"
Expand Down
1 change: 0 additions & 1 deletion accessor/reference_helper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include <type_traits>
#include <utility>


#include "utils.hpp"


Expand Down
1 change: 0 additions & 1 deletion accessor/row_major.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include <array>
#include <cinttypes>


#include "accessor_helper.hpp"
#include "range.hpp"
#include "utils.hpp"
Expand Down
1 change: 0 additions & 1 deletion accessor/scaled_reduced_row_major.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include <type_traits>
#include <utility>


#include "accessor_helper.hpp"
#include "index_span.hpp"
#include "range.hpp"
Expand Down
1 change: 0 additions & 1 deletion accessor/scaled_reduced_row_major_reference.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

#include <type_traits>


#include "math.hpp"
#include "reference_helper.hpp"
#include "utils.hpp"
Expand Down
4 changes: 1 addition & 3 deletions benchmark/blas/blas.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@
//
// SPDX-License-Identifier: BSD-3-Clause

#include <ginkgo/ginkgo.hpp>


#include <cstdlib>
#include <iomanip>
#include <iostream>

#include <ginkgo/ginkgo.hpp>

#include "benchmark/blas/blas_common.hpp"
#include "benchmark/utils/general.hpp"
Expand Down
4 changes: 1 addition & 3 deletions benchmark/blas/blas_common.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
//
// SPDX-License-Identifier: BSD-3-Clause

#include <ginkgo/ginkgo.hpp>


#include <algorithm>
#include <chrono>
#include <cstdlib>
Expand All @@ -13,6 +10,7 @@
#include <iostream>
#include <typeinfo>

#include <ginkgo/ginkgo.hpp>

#include "benchmark/utils/general.hpp"
#include "benchmark/utils/iteration_control.hpp"
Expand Down
5 changes: 2 additions & 3 deletions benchmark/blas/distributed/multi_vector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@
//
// SPDX-License-Identifier: BSD-3-Clause

#include <ginkgo/ginkgo.hpp>


#include <cstdlib>
#include <iomanip>
#include <iostream>

#include <ginkgo/ginkgo.hpp>


#define GKO_BENCHMARK_DISTRIBUTED

Expand Down
4 changes: 1 addition & 3 deletions benchmark/conversion/conversion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
//
// SPDX-License-Identifier: BSD-3-Clause

#include <ginkgo/ginkgo.hpp>


#include <algorithm>
#include <chrono>
#include <cstdlib>
Expand All @@ -14,6 +11,7 @@
#include <iostream>
#include <typeinfo>

#include <ginkgo/ginkgo.hpp>

#include "benchmark/utils/formats.hpp"
#include "benchmark/utils/general.hpp"
Expand Down
Loading
Loading