-
Notifications
You must be signed in to change notification settings - Fork 143
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
Automate C code formatting #391
Conversation
Please review the code style and provide feedback. You'd better just check out my branch and inspect it yourself in your development environment:
Meanwhile I'll be negotiating behavior differences between clang-format 6.0 and 8.0... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code is much better now. just one remark that will be better to avoid oneline if statements without braces because statistic tells that it a good place for human mistakes
@Lagovas I fully agree with you on that one-line ifs also need braces. Unfortunately, clang-format does not seem to be able to insert them. I looked for a couple of other formatters like astyle, but they were worse then clang-format in other places. Another option that I have in mind is trying clang-tidy, maybe it will work better. |
Let's introduce an automatic code style formatting and checks for Themis. We use clang-format and clang-tidy tools as they are generally available and gives reasonable results. Introduce two new targets for Makefile: - make fmt - make fmt_check "fmt" is intended for humans to reformat the code before submitting changes for review. Currently it formats only C code but it may be extended to other languages as well. "fmt_check" is intended for CI to do format checks to ensure that the code is formatted accordingly. Unfortunately, clang-format does not have a convenient check mode so we hack one with git diff. clang-tidy requires the compilation flags so we integrate the format checking into the build pipeline. It is based on a standalone tool which can be integrated into developer's IDE if necessary. We start by formatting only Soter and Themis source code.
Drop commented out code that cause problems with formatting. clang-tidy and clang-format conflict in line-wrapping this part.
Actually reformat the whole code base in accordance with the style.
Enforce the new code style during CI builds. We use a separate job so that style errors are reported separately from failed tests. We also check both flavors as they use different Soter sources.
@Lagovas @secumod I have updated the PR to use the new approach to formatting. It is now uses Makefile targets to guide formatting and static checks. This enables nice progress and error reports. The main targets are the same:
Plus there is a new one:
The formatting logic is placed into a new script We now also use clang-tidy tool to apply source-level fixups (e.g., inserting braces around one-line conditional statements). The ruleset is fairly minimal now but can be extended later. I have also limited the changes in this PR to main source code of Soter and Themis. We'll update other places later when we settle on the code style. This PR currently includes commits from #392 which are necessary for the tooling to work. They will be removed when #392 is merged into master. |
@@ -0,0 +1,4 @@ | |||
--- | |||
Checks: 'readability-*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are more check available, but I'd like to enable them later in separate pull requests as they can change the code semantics.
src/soter/boringssl/soter.mk
Outdated
@@ -15,6 +15,10 @@ | |||
# | |||
|
|||
SOTER_SRC += $(wildcard $(CRYPTO_ENGINE)/*.c) | |||
SOTER_AUD_SRC += $(wildcard $(CRYPTO_ENGINE)/*.c) | |||
SOTER_AUD_SRC += $(wildcard $(CRYPTO_ENGINE)/*.h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turned out that BoringSSL build for audit did not include BoringSSL code for Soter. I've added it here.
Initialize submodules to pull BoringSSL source code. For some reason CircleCI does not do that for us.
src/soter/boringssl/soter.mk
Outdated
SOTER_AUD_SRC += $(wildcard $(CRYPTO_ENGINE)/*.c) | ||
SOTER_AUD_SRC += $(wildcard $(CRYPTO_ENGINE)/*.h) | ||
SOTER_FMT_SRC += $(wildcard $(CRYPTO_ENGINE)/*.c) | ||
SOTER_FMT_SRC += $(wildcard $(CRYPTO_ENGINE)/*.h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we doing all these variable copies for the same data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency. Should I just replace this with the following?
CRYPTO_ENGINE_SOURCES = $(wildcard $(CRYPTO_ENGINE)/*.c)
CRYPTO_ENGINE_HEADERS = $(wildcard $(CRYPTO_ENGINE)/*.h)
SOTER_SRC += $(CRYPTO_ENGINE_SOURCES)
SOTER_AUD_SRC += $(CRYPTO_ENGINE_SOURCES) $(CRYPTO_ENGINE_HEADERS)
SOTER_FMT_SRC += $(CRYPTO_ENGINE_SOURCES) $(CRYPTO_ENGINE_HEADERS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have some variables, why create more aggregate variables? Our format target can just depend on $(SOTER_SRC) $(CRYPTO_ENGINE_SOURCES) ..
Makefile
Outdated
FMT_FILES += $(THEMIS_FMT) | ||
FMT_FILES += $(SOTER_FMT) | ||
|
||
$(OBJ_PATH)/%.fmt: CMD = CFLAGS='$(CFLAGS)' ./scripts/fmt.sh $< $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I personally don't favour hiding the code in a separate script (unless it is too complicated), because it just creates another level of indirection
- let's keep the language suffix, so we know which files are being formatted (
.c
for formatting C code). Later we should reuse the framework to format other languages (.go
and.rs
at least).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put it into a script because we'll probably have to write almost the same Makefile targets in what seems like 5+ places (JNI wrapper, Soter and Themis tests, ThemisPP code and tests, JSThemis code).
While the C compiler is easy to use via $(CC)
, this formatting abomination is not. And it has two operation modes with different command lines. So I used a script to avoid duplicating the code.
The same logic applies to suffixes. We'll have to format at least *.c, *.h, *.hpp, *.cpp files. I would not like to write the same targets four times. The script could guess the language by the file extension and run an appropriate formatter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- you can write them in one place and just
include whatever.mk
where needed - you can have a single command for multiple extensions, if they are processed the same:
%.foo %.bar:
<some common processing for both *.foo and *.bar>
%.baz:
<some other command for baz>
Remove custom installation of clang-format and clang-tidy since they are a part of the Docker image from now on.
Avoid copy-pasting wildcard substitutions all around, put them into variable and make the file lists more compact that way.
Just inline invocations of clang tools into the Makefile, do not use an intermediate shell script as that complicates inspection. Also, limit the effect to *.c and *.h files in $(SRC_PATH). We can extend the formatting later to other file types. Now that we don't have a configurable script we have to duplicate the fixups and checks. However, that allows us to be more precise with checks and avoid interference between fixups and checks. With that we no longer need the "fmt_clean" target, just a regular "clean" will do.
@secumod I've updated the PR, please give it a look.
I still don't quite like how we have to duplicate these lines: $(OBJ_PATH)/%.fmt_fixup: $(SRC_PATH)/%
@mkdir -p $(@D)
@echo -n "fixup $< "
@$(BUILD_CMD_)
$(OBJ_PATH)/%.fmt_check: $(SRC_PATH)/%
@mkdir -p $(@D)
@echo -n "check $< "
@$(BUILD_CMD_) However, I don't see how it can be avoided without doing some crazy macro stuff with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from first glance. I suggest the following: break this PR into two (or three):
- one that introduces the changes to the build system and format targets
- the other one, which actually is the result of
make fmt
- enforcing the format in CI (maybe merged with the previous one - don't have a strong opinion here)
You think you duplicate, because you hide the difference by redefining the |
Well, even without $(OBJ_PATH)/%.fmt_fixup: $(SRC_PATH)/%
@$(CLANG_TIDY) -fix $< -- $(CFLAGS) 2>/dev/null
@$(CLANG_FORMAT) -i $<
@touch $@
$(OBJ_PATH)/%.fmt_check: $(SRC_PATH)/%
@$(CLANG_FORMAT) $< | diff -u $< -
@$(CLANG_TIDY) $< -- $(CFLAGS) 2>/dev/null
@touch $@ Though the reason for duplication will be more apparent in this case, as they really do different actions. |
oh, |
Oh, even more targets as we should apply clang tools only to C files: $(OBJ_PATH)/%.c.fmt_fixup: $(SRC_PATH)/%.c
# fixup
$(OBJ_PATH)/%.h.fmt_fixup: $(SRC_PATH)/%.h
# fixup
$(OBJ_PATH)/%.c.fmt_check: $(SRC_PATH)/%.c
# check
$(OBJ_PATH)/%.h.fmt_check: $(SRC_PATH)/%.h
# check with fixup and check actions being exactly the same. Plus we'll have to write the same lines for every other combination of source file and object-file... Though, I guess we can avoid this particular pain point by not reusing the object file hierarchy for checks and just mirroring the source file locations, using the rules like: $(CHECK_PATH)/%.h.fmt_check: %.h
# check C header
$(CHECK_PATH)/%.c.fmt_check: %.c
# check C source and generating targets like this: -THEMIS_FMT_FIXUP = $(patsubst $(SRC_PATH)/%,$(OBJ_PATH)/%.fmt_fixup,$(THEMIS_FMT_SRC))
+THEMIS_FMT_FIXUP = $(patsubst %,$(CHECK_PATH)/%.fmt_fixup,$(THEMIS_FMT_SRC)) |
Anyway... This PR ended up being too complex to review. I have split it into three parts (as it should be done from the starts). They include all changes discussed here rebased on top of the current master branch. |
Let's introduce an automatic code style formatting and checks for Themis. We use clang-format as this tool is generally available and gives reasonable results.
Introduce two new targets for Makefile:
make fmt
make fmt_check
fmt
is intended for humans to reformat the code before submitting changes for review. Currently it formats only C code but it may be extended to other languages as well.fmt_check
is intended for CI to do format checks to ensure that the code is formatted accordingly. Unfortunately, clang-format does not have a convenient check mode so we hack one with git diff.Then actually reformat the whole code base in accordance with the style. Enforce the new code style during CI builds. We use a separate job so that style errors are reported separately from failed tests.
This pull includes the formatting changes as well to ensure that the build is green. There is no point in reviewing the restyling commit itself, focus on the other ones.
Do not merge this pull request via GitHub interface, I'll do it manually to preserve the authorship information as I don't want to be credited for authoring half of this repository.
Closes #270