Add a 'clang-format' build target #612
Conversation
If clang-format can be found at config time, then a build target will
be established that allows you to `make clang-format`.
Running this does not cause any actual compiles, but will reformat all
the .h and .cpp files in the code base, according to the .clang-format
configuration.
Cache variables CLANG_FORMAT_INCLUDES and CLANG_FORMAT_EXCLUDES
provides patterns for any files to include (default: `"*.h;*.cpp"`) or
exclude, respectively. Files that you are sure should never be
reformatted can be added explicitly to the CLANG_FORMAT_EXCLUDES
default in clang-format.cmake.
Note that there will be some source modules which, in whole or in part,
you know you don't want to be reformatted (sometimes a nonstandard
formatting is much clearer than anything clang-format can be coaxed into
doing, such is often the case for tables). In that case you can turn it
off with the comment
// clang-format off
... code that is excluded from reformatting ...
// clang-format on
The CMake implementation of finding clang-format and adding this target
was largely borrowed from that in OpenImageIO's build system (also the
same BSD-3-Clause license).
This patch only establishes the build-time target so that you can run
it by hand, but does not (in this commit) reformat any of the
source. There are a LOT of diffs! We may want to fine-tune the
.clang-format for a while before we take the plunge of actually
checking in the reformatted code and then enforcing the formatting
thereafter. (Pro tip: run `make clang-format` only when you have no
uncommitted edits. If you then don't like the formatting it imposes,
you can back out of it all with `git checkout HEAD --force`.)
CMakeLists.txt
Outdated
| @@ -24,6 +24,10 @@ endif() | |||
|
|
|||
| project(OpenEXRMetaProject) | |||
|
|
|||
| # Search for custom modules in our cmake directory | |||
| list (APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake") | |||
There was a problem hiding this comment.
I don't know why, but I had been avoiding changing the module path, and just including cmake/foo. I could have sworn I saw somewhere that to support other projects including your project as a sub-project, you shouldn't adjust the module path. But now I can't find that reference. Either, way, is there a particular reason to add the path vs. just including directly?
There was a problem hiding this comment.
Oh, sure. I was just trying to keep the include directives clean, but if you'd rather they be explicit, that's fine.
There was a problem hiding this comment.
pushed an update to address this
| file (COPY ${CMAKE_CURRENT_SOURCE_DIR}/.clang-format | ||
| DESTINATION ${CMAKE_CURRENT_BINARY_DIR}) | ||
| add_custom_target (clang-format | ||
| COMMAND "${CLANG_FORMAT_EXE}" -i -style=file ${FILES_TO_FORMAT} ) |
There was a problem hiding this comment.
hmmm, this is probably ok, although currently the filelist turns into ~26k worth of text, so some people may have issues running this on shells that have a limited command line text. If this comes up, one way to reduce that would be to make this a per-library command with a global pseudo target that depends on each of the per-target ones. Then you could also do something like get_target_property(target_sources ${target} SOURCES) to extract rather than having to do the GLOB_RECURSE above.
Which if we start adding pybind11 as a sub-project or something may be a good idea to do (to constrain what files clang-format runs against). Up to you, don't want to slow this down getting in - just thinking of ways to improve it.
There was a problem hiding this comment.
I think I get what you're saying. How badly do you want that done now? This will probably all get some revision when we get around to really trying this out and reformatting things. My main goal for this patch was just to give the current primary developers a single simple command to reformat the codebase.
If clang-format can be found at config time, then a build target will
be established that allows you to
make clang-format.Running this does not cause any actual compiles, but will reformat all
the .h and .cpp files in the code base, according to the .clang-format
configuration.
Cache variables CLANG_FORMAT_INCLUDES and CLANG_FORMAT_EXCLUDES
provides patterns for any files to include (default:
"*.h;*.cpp") orexclude, respectively. Files that you are sure should never be
reformatted can be added explicitly to the CLANG_FORMAT_EXCLUDES
default in clang-format.cmake.
Note that there will be some source modules which, in whole or in part,
you know you don't want to be reformatted (sometimes a nonstandard
formatting is much clearer than anything clang-format can be coaxed into
doing, such is often the case for tables). In that case you can turn it
off with the comment
The CMake implementation of finding clang-format and adding this target
was largely borrowed from that in OpenImageIO's build system (also the
same BSD-3-Clause license).
This patch only establishes the build-time target so that you can run
it by hand, but does not (in this commit) reformat any of the
source. There are a LOT of diffs! We may want to fine-tune the
.clang-format for a while before we take the plunge of actually
checking in the reformatted code and then enforcing the formatting
thereafter. (Pro tip: run
make clang-formatonly when you have nouncommitted edits. If you then don't like the formatting it imposes,
you can back out of it all with
git checkout HEAD --force.)