search

bcgsc / btl_bloomfilter

The BTL C/C++ Common bloom filters for bioinformatics projects, as well as any APIs created for other programming languages.
GNU General Public License v3.0
18 stars 4 forks source link

Add clang format and proper white space formatted C files #11

Closed jwcodee closed 5 years ago

JustinChu commented 5 years ago

I'm against autoformatting everything. Mostly because it muddles the line history and makes it difficult to confirm the version various external headerfiles from other projects, like ntHash. These header files should be treated as an external library and untouched, if the are changed they should be changed in the ntHash repo and copied here first.

Using a linter is one thing and so is using a formatter, but it should be done for the sake of readability in a case by case basis. In some cases the auto-formatter doesn't do a good job formatting some novel parts of the code.

sjackman commented 5 years ago

I'm not aware of a white space linter for C++. Clang doesn't have one as far as I know, so we're using clang-format. It's difficult to review PRs when the white space is inconsistent and all over the map. I'm strongly in favour of using clang-format to ensure consistent white space. The .clang-format configuration file can be tweaked if you disagree with the way it formats a particular construct.

These header files should be treated as an external library and untouched, if the are changed they should be changed in the ntHash repo and copied here first.

I agree with this statement. Vendored source code should be left untouched, or first updated upstream. @jowong4 Please move the vendored source files to a vendor/ directory in the top-level.

@mohamadi Would you like @jowong4 to open a PR to add clang-format to the continuous integration (CI) using Azure Pipelines for ntHash?

sjackman commented 5 years ago

@JustinChu Fixing up the white space by hand based on a failed CI run is super tedious. You can run clang-format -i *.cc *.h to correct all the white space in your local files, or you can run make clang-format and move the *.fixed files to overwrite your local files.

JustinChu commented 5 years ago

After reading up on using a formatter on all files in a project continuously I've changed my tune on this a bit. However I still think there are some problems to our approach. For instance, I wish there was an easier way to ensure the binaries for clang-format were automatically retrived or something as different versions of clang-format yield different results. A wrapper script was used in this project for this purpose and I wonder if there was a more lightweight general purpose script for this. The wrapper script serves other purposes too like parallelism.

sjackman commented 5 years ago

After reading up on using a formatter on all files in a project continuously I've changed my tune on this a bit.

Thank you for pointing me to this article! It's excellent.

However I still think there are some problems to our approach. For instance, I wish there was an easier way to ensure the binaries for clang-format were automatically retrived or something as different versions of clang-format yield different results.

I suggest that we standardize on the version provided by Homebrew, installed at /gsc/btl/linuxbrew/bin/clang-format. It was version 6.0.1, and I'm upgrading it now to version 8.0.0.

Our CI is currently configured to use clang-format 7. We can update our CI to use version 8. So far I haven't seen any difference between version 7 and 8.

jwcodee commented 5 years ago

I will add the vendor directory with the unchanged nthash code in the next pull request

sjackman commented 5 years ago

Would you like to open a PR to add clang-format to https://github.com/bcgsc/nthash ?