Skip to content
This repository has been archived by the owner on Jan 30, 2023. It is now read-only.

issue #25 - format libprio with clang-format, using Mozilla style #26

Merged
merged 3 commits into from
Aug 29, 2018

Conversation

rhelmer
Copy link
Contributor

@rhelmer rhelmer commented Aug 28, 2018

This reformats existing source clang-format using Mozilla style, and adds a script to the Travis config which will print a diff in the build output with any new changes that do not match the style.

@rhelmer rhelmer force-pushed the issue-25-use-clang-format branch 3 times, most recently from beeaa69 to e985342 Compare August 28, 2018 20:31
@rhelmer rhelmer requested a review from henrycg August 28, 2018 20:33
@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 28, 2018

Yeesh, finally got it to pass my own test :) There's a super minor difference (style for commented out code) between my local clang-format and the one on travis.. I bet it's just an older version of clang in the Ubuntu distro travis uses.

In any case I think the important thing is consistency, and this particular difference seems pretty meaningless.

@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 28, 2018

Oh, so Travis compiles and runs ptest and that all seems fine with these changes, I also ran browser-test just for good measure.

@henrycg
Copy link
Collaborator

henrycg commented Aug 28, 2018

Cool! If I run scripts/clang-format.sh on Linux (clang-format 6.0.0), I get a bunch of errors:

*** The following differences were found between the code to commit and the clang-format rules:

--- a/include/mprio.h	2018-08-28 13:35:42.490744697 -0700
+++ b/include/mprio.h	2018-08-28 13:40:59.738211269 -0700
@@ -10,7 +10,8 @@
 #define __PRIO_H__
 
 #ifdef __cplusplus
-extern "C" {
+extern "C"
+{
 #endif
 
 #include <msgpack.h>
@@ -22,7 +23,7 @@
 
 /* Seed for a pseudo-random generator (PRG). */
 #define PRG_SEED_LENGTH AES_128_KEY_LENGTH
-typedef unsigned char PrioPRGSeed[PRG_SEED_LENGTH];
+  typedef unsigned char PrioPRGSeed[PRG_SEED_LENGTH];
 
 /* Length of a raw curve25519 public key, in bytes. */
 #define CURVE25519_KEY_LEN 32
@@ -30,287 +31,258 @@
 /* Length of a hex-encoded curve25519 public key, in bytes. */
 #define CURVE25519_KEY_LEN_HEX 64

... and many more. Is this what I should expect to see? Or is my clang-format different from yours?

@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 28, 2018

@henrycg hm yeah mine is slightly different too... I suspect it's just that they are different versions of clang-format, I'll dig into it a bit before we merge as it'd be nice to be able to run locally and get more consistency than this...

@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 29, 2018

@henrycg just to double-check - did you pull this whole branch, or just the scripts/clang-format.sh script?

There's a .clang_format file in the root that has all the style rules which is what I want to make sure about :) Pretty sure the default style is LLVM.

My clang-format (and presumably the one on the Travis VM which is Ubuntu LTS I think by default) is much older than yours:
clang-format version 4.0.1 (tags/RELEASE_401/final)

@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 29, 2018

OK so I took around online and apparently this is a pretty widespread problem (different clang-format versions having lots of differences :/)

I think the way to resolve is to just have a supported version, in the Ubuntu repos I see there are tons of versions available, presumably because of this problem. The latest on Homebrew is 4.0 so I'd suggest we go with that... going to see if I can force that on Travis.

@rhelmer rhelmer force-pushed the issue-25-use-clang-format branch 3 times, most recently from 78f8dd1 to ac33f15 Compare August 29, 2018 20:16
@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 29, 2018

@henrycg ok there we go... so. Travis only supports Trusty (the previous LTS before Bionic, still supported until next April), and the latest clang-format there is 3.9

Do you have a clang-format-3.9 package available in the distro you're using? If so please install that and try the ./scripts/clang-format.sh script, see if it does the right thing.

Personally I think staying compatible with the oldest still-supported LTS is a pretty decent target, but we could scuttle the whole automated-checking if you think it's too much of a burden.

@henrycg
Copy link
Collaborator

henrycg commented Aug 29, 2018

Yes, this works for me now. I'm able to get clang-format-3.9 via apt, so this setup looks good. If the automatic checking turns out to be a burden, we can revisit it later on.

@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 29, 2018

Great! I actually don't care that much about exactly what the code style is (for the most part), but I find having it be automated puts an end to unproductive debates :)

@rhelmer rhelmer merged commit ded8b10 into mozilla:master Aug 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants