Page MenuHomePhabricator

Automatically add missing braces
ClosedPublic

Authored by Fabien on Jan 30 2020, 22:26.

Details

Summary

This diff adds a check to clang-tidy for adding the braces automatically.
Configuration files are added to the libraries (leveldb, secp256k1,
univalue) to avoid formatting them.

Depends on D5138.

Test Plan

Add some missing brace somewhere in a source file if D5120 is not landed yet.

cmake -GNinja ..
ninja

Check that the files are updated with braces at build time.

Reset the file to have missing braces, then:

cmake -GNinja .. -DENABLE_CLANG_TIDY=OFF
ninja

Check that clang-tidy is not run.

Run the Gitian builds.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
clang_tidy
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9218
Build 16382: Default Diff Build & Tests
Build 16381: arc lint + arc unit

Event Timeline

Snippet of first build failure:

[22:26:53] :	 [Step 1/1] -- Looking for getifaddrs
[22:26:53] :	 [Step 1/1] -- Looking for getifaddrs - found
[22:26:53] :	 [Step 1/1] -- Looking for freeifaddrs
[22:26:53] :	 [Step 1/1] -- Looking for freeifaddrs - found
[22:26:53] :	 [Step 1/1] -- Performing Test HAVE_SYS_GETRANDOM
[22:26:53] :	 [Step 1/1] -- Performing Test HAVE_SYS_GETRANDOM - Success
[22:26:53] :	 [Step 1/1] -- Performing Test HAVE_SYSCTL_ARND
[22:26:53] :	 [Step 1/1] -- Performing Test HAVE_SYSCTL_ARND - Failed
[22:26:53] :	 [Step 1/1] -- Performing Test CHAR_EQUALS_INT8
[22:26:53] :	 [Step 1/1] -- Performing Test CHAR_EQUALS_INT8 - Failed
[22:26:53] :	 [Step 1/1] -- Performing Test HAVE_LARGE_FILE_SUPPORT
[22:26:54] :	 [Step 1/1] -- Performing Test HAVE_LARGE_FILE_SUPPORT - Success
[22:26:54] :	 [Step 1/1] -- Performing Test HAVE_FUNC_ATTRIBUTE_VISIBILITY
[22:26:54] :	 [Step 1/1] -- Performing Test HAVE_FUNC_ATTRIBUTE_VISIBILITY - Success
[22:26:54] :	 [Step 1/1] -- Performing Test HAVE_FUNC_ATTRIBUTE_DLLEXPORT
[22:26:54] :	 [Step 1/1] -- Performing Test HAVE_FUNC_ATTRIBUTE_DLLEXPORT - Failed
[22:26:54] :	 [Step 1/1] -- Checking prototype __fdelt_warn for FDELT_PROTOTYPE_LONG_UNSIGNED_INT - False
[22:26:54] :	 [Step 1/1] -- Looking for EVP_MD_CTX_new
[22:26:54] :	 [Step 1/1] -- Looking for EVP_MD_CTX_new - found
[22:26:54]W:	 [Step 1/1] CMake Error at cmake/modules/DoOrFail.cmake:4 (message):
[22:26:54]W:	 [Step 1/1]   Failed to find program
[22:26:54]W:	 [Step 1/1]   [clang-tidy;clang-tidy-8;clang-tidy-9;clang-tidy-10], please make sure that
[22:26:54]W:	 [Step 1/1]   it is installed and reachable through the system PATH.
[22:26:54]W:	 [Step 1/1] Call Stack (most recent call first):
[22:26:54]W:	 [Step 1/1]   cmake/modules/ClangTidy.cmake:5 (find_program_or_fail)
[22:26:54]W:	 [Step 1/1]   src/crypto/CMakeLists.txt:19 (include)
[22:26:54]W:	 [Step 1/1] 
[22:26:54]W:	 [Step 1/1] 
[22:26:54] :	 [Step 1/1] -- Configuring incomplete, errors occurred!
[22:26:54] :	 [Step 1/1] See also "/home/teamcity/buildAgent/work/c4a5708f2bae7929/build/native/CMakeFiles/CMakeOutput.log".
[22:26:54] :	 [Step 1/1] See also "/home/teamcity/buildAgent/work/c4a5708f2bae7929/build/native/CMakeFiles/CMakeError.log".
[22:26:54] :	 [Step 1/1] [56/400] Building CXX object src/leveldb/CMakeFiles/leveldb.dir/port/port_posix.cc.o
[22:26:54] :	 [Step 1/1] ../src/leveldb/port/port_posix.cc: In function 'bool leveldb::port::HasAcceleratedCRC32C()':
[22:26:54] :	 [Step 1/1] ../src/leveldb/port/port_posix.cc:60:15: warning: 'ecx' may be used uninitialized in this function [-Wmaybe-uninitialized]
[22:26:54] :	 [Step 1/1]    return (ecx & (1 << 20)) != 0;
[22:26:54] :	 [Step 1/1]           ~~~~~^~~~~~~~~~~~
[22:26:54] :	 [Step 1/1] [102/400] Generating bitcoin_hu.qm
[22:26:54] :	 [Step 1/1] Removed plural forms as the target language has less forms.
[22:26:54] :	 [Step 1/1] If this sounds wrong, possibly the target language is not set or recognized.
[22:26:54] :	 [Step 1/1] [126/400] Generating bitcoin_pl.qm
[22:26:54] :	 [Step 1/1] Removed plural forms as the target language has less forms.
[22:26:54] :	 [Step 1/1] If this sounds wrong, possibly the target language is not set or recognized.
[22:26:54] :	 [Step 1/1] [137/400] Generating bitcoin_ru.qm
[22:26:54] :	 [Step 1/1] Removed plural forms as the target language has less forms.
[22:26:54] :	 [Step 1/1] If this sounds wrong, possibly the target language is not set or recognized.
[22:26:54] :	 [Step 1/1] [149/400] Generating bitcoin_tr.qm
[22:26:54] :	 [Step 1/1] Removed plural forms as the target language has less forms.
[22:26:54] :	 [Step 1/1] If this sounds wrong, possibly the target language is not set or recognized.
[22:26:54] :	 [Step 1/1] [185/400] Linking CXX static library src/zmq/libzmq.a
[22:26:54] :	 [Step 1/1] FAILED: native/CMakeCache.txt 
[22:26:54] :	 [Step 1/1] cd /home/teamcity/buildAgent/work/c4a5708f2bae7929/build && /home/teamcity/buildAgent/work/c4a5708f2bae7929/build/config/run_native_cmake.sh
[22:26:54] :	 [Step 1/1] ninja: build stopped: subcommand failed.
[22:26:54] :	 [Step 1/1] *** Output of /tmp/sanitizer_logs/*.log.* ***
[22:26:54]W:	 [Step 1/1] ++ print_sanitizers_log
[22:26:54]W:	 [Step 1/1] ++ for log in "${SAN_LOG_DIR}"/*.log.*
[22:26:54]W:	 [Step 1/1] ++ echo '*** Output of /tmp/sanitizer_logs/*.log.* ***'
[22:26:54]W:	 [Step 1/1] ++ cat '/tmp/sanitizer_logs/*.log.*'
[22:26:54]W:	 [Step 1/1] cat: '/tmp/sanitizer_logs/*.log.*': No such file or directory
[22:26:54]W:	 [Step 1/1] Process exited with code 1
[22:26:54]E:	 [Step 1/1] Process exited with code 1 (Step: Command Line)
Fabien planned changes to this revision.Jan 30 2020, 22:30

Disable for native builds.

deadalnix requested changes to this revision.Jan 31 2020, 01:55
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/CMakeLists.txt
581 ↗(On Diff #15907)

This is clearly not going to scale if we need to add it to every single target.

This revision now requires changes to proceed.Jan 31 2020, 01:55

Several improvements:

  • Use an exclusion mechanism instead of inclusion to avoid duplication
  • Forward the clang-tidy option to the native build, so the native source files get formatted
  • Rebase on top of D5121 to avoid formatting generated files
  • Fix CONTRIBUTING.md, improve OSX instructions
Fabien edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Jan 31 2020, 18:09
deadalnix added inline comments.
CONTRIBUTING.md
130

Add a line break.

148

It's be great if we gave a people a way to do thing either with brew or npm, but not a mixture of both.

cmake/modules/ClangTidy.cmake
23

It would be preferable to use a .clang-tidy file instead of forcing anyone to know the build system internal soup to be able to do anything with clang-tidy.

With some luck, it even fix your problem with generated files, because they'll be in the build directory instead of the source directory.

src/CMakeLists.txt
202

That is really ugly. Having a clang-tidy file in these directories that disable the check that fails would be more beneficial.

This revision now requires changes to proceed.Jan 31 2020, 18:09
Fabien planned changes to this revision.Jan 31 2020, 19:35
Fabien added inline comments.
CONTRIBUTING.md
148

There is no npm package for clang-tidy, and no brew formulae for clang-format-8 :(.
Getting the pre-built binaries is maybe a better and more consistent solution. I'll look into this.

cmake/modules/ClangTidy.cmake
23

This was the first thing I tried but I faced some limitations and finally I gave up.
The main issue is that you cannot disable all the checks (neither in a config file not through command line) or clang-tidy will return an error, so the only way is to not run clang-tidy on the files for which you don't want to enable any check.
You can think of some kind of file exclusion, and there is an option for that but it does not support regex and is only available on the command line, not in a configuration file.

There is one last possibility that I didn't explore: finding a rule that is green for the whole codebase, and which we want to enforce. Doing so will allow to use the files, as there will always be one rule activated. I'll see if there is such a rule, and update the diff.

Separate clang-tidy integration in D5138 and rebase on top.
This diff now simply enable the rule and adds the config files
for disabling it in the libraries.

This revision is now accepted and ready to land.Feb 3 2020, 17:45
This revision was automatically updated to reflect the committed changes.