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

Add clang-format #1701

Merged
merged 9 commits into from Feb 5, 2019

Conversation

Projects
None yet
3 participants
@palmerlao
Copy link
Contributor

palmerlao commented Jan 3, 2019

This is an attempt to fix #1692.

(I think) you can reproduce most of this PR by:

  • Dumping a Google-style .clang-format file:
    clang-format -style=Google -dump-config > .clang-format
  • Making a few arbitrary changes to somewhat suit the project's current style better:
~/vowpal_wabbit$ diff .clang-format <(clang-format -style=Google -dump-config)
14,15c14,15
< AllowShortFunctionsOnASingleLine: None
< AllowShortIfStatementsOnASingleLine: false
---
> AllowShortFunctionsOnASingleLine: All
> AllowShortIfStatementsOnASingleLine: true
36c36
< BreakBeforeBraces: Allman
---
> BreakBeforeBraces: Attach
39c39
< ColumnLimit:     120
---
> ColumnLimit:     80
  • Format everything under vowpalwabbit/:
    find vowpalwabbit -type f -name '*.cc' -o -name '*.h' | xargs clang-format -i

It also adds onto the end of the script that the Linux CI uses to check if running clang-format would result in any changes.

@palmerlao palmerlao changed the title Add clang-format WIP: Add clang-format Jan 3, 2019

@jackgerrits
Copy link
Member

jackgerrits left a comment

I would like to see some small changes around alignment. I'm going to block this PR because we need the refactor change merged before this otherwise the conflicts will be terrible.

Show resolved Hide resolved .clang-format Outdated
Show resolved Hide resolved vowpalwabbit/OjaNewton.cc Outdated
Show resolved Hide resolved vowpalwabbit/action_score.h Outdated
float* shared_weights = (float*)mmap(0, (length << _stride_shift) * sizeof(float),
PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);
float* shared_weights = (float*)mmap(0, (length << _stride_shift) * sizeof(float), PROT_READ | PROT_WRITE,
MAP_SHARED | MAP_ANONYMOUS, -1, 0);

This comment has been minimized.

@jackgerrits

jackgerrits Jan 3, 2019

Member

wrapped lines should not be aligned, just indented

This comment has been minimized.

@jackgerrits

jackgerrits Jan 4, 2019

Member

I don't think this change is wanted. Is there an option that will mean this line isn't changed. (The existing style is correct)

This comment has been minimized.

@palmerlao

palmerlao Jan 4, 2019

Author Contributor

I'm not sure it can be verbatim the same. The original version starts a new unaligned line for the third argument. Using AlignAfterOpenBracket: DontAlign for some reason forces it to start a new indented line with the first argument.

The other alternatives for the option I used (AlignAfterOpenBracket) to get indentation instead of alignment are Align (the intermediate change we don't want) and AlwaysBreak (I expect it will look the same as now).

This comment has been minimized.

@jackgerrits

jackgerrits Jan 4, 2019

Member

It probably forces it because the line is too long, not sure. AlignAfterOpenBracket: DontAlign seems best?

Show resolved Hide resolved vowpalwabbit/best_constant.cc Outdated
Show resolved Hide resolved vowpalwabbit/feature_group.h Outdated
@palmerlao

This comment has been minimized.

Copy link
Contributor Author

palmerlao commented Jan 4, 2019

I made a change to the Dockerfile in this repo to add clang-format, which I hope will be reflected in vowpalwabbit/travis-base, but it's not clear to me that this happens automatically. Would it be better to cut two PRs for this, one to install clang-format in the CI image, and another to apply the formatting changes?

@jackgerrits

This comment has been minimized.

Copy link
Member

jackgerrits commented Jan 4, 2019

The dockerfile isn't currently auto-rebuilt. I'd love to get around to that sometime soon. If you could make the changes in both the dockerfile and the test script and I'll remove it from the test script when I update the image.

Show resolved Hide resolved .clang-format
Show resolved Hide resolved .clang-format

@palmerlao palmerlao force-pushed the palmerlao:dev/palmerlao/add-clang-format branch from 9dda917 to d78b9bc Jan 4, 2019

@palmerlao palmerlao changed the title WIP: Add clang-format Add clang-format Jan 4, 2019

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Jan 16, 2019

With #1711 finally in, I'd like to get this one in. Is it easy to rerun the reformatting to deal with the merge conflicts created?

@palmerlao

This comment has been minimized.

Copy link
Contributor Author

palmerlao commented Jan 16, 2019

@JohnLangford I hope it is pretty easy. I can try it soon (tonight or otherwise within a few days).

@palmerlao palmerlao force-pushed the palmerlao:dev/palmerlao/add-clang-format branch 3 times, most recently from 6518250 to c48b575 Jan 17, 2019

@palmerlao

This comment has been minimized.

Copy link
Contributor Author

palmerlao commented Jan 17, 2019

Apologies for the git bungling. Think this is ready for another pass

.add(make_option("alpha", ON->alpha).default_value(1.f).help("mutiplicative constant for indentiy"))
.add(make_option("alpha_inverse", alpha_inverse).help("one over alpha, similar to learning rate"))
.add(make_option("learning_rate_cnt", ON->learning_rate_cnt)
.default_value(2.f)

This comment has been minimized.

@JohnLangford

JohnLangford Jan 17, 2019

Collaborator

Why is .default_value shifted to new line? Any way to avoid that?

bool not_null() { return (_weight_mask > 0 && !_map.empty()); }
public:
sparse_parameters(size_t length, uint32_t stride_shift = 0)
: _map(),

This comment has been minimized.

@JohnLangford

JohnLangford Jan 17, 2019

Collaborator

Can this take fewer lines?

This comment has been minimized.

@jackgerrits

jackgerrits Jan 17, 2019

Member

It it doesn’t all fit on one line then it splits across multiple. I’d prefer this to be one per line (as is) rather than bin packed

This comment has been minimized.

@JohnLangford

JohnLangford Jan 18, 2019

Collaborator

is bin packed an option? The same thing is happening to option declarations which is a hefty chunk of the extra 2k lines of code.

This comment has been minimized.

@jackgerrits

jackgerrits Jan 18, 2019

Member

I think the fact this is split across lines is good, much easier to read. The option declarations also are very long lines, so it is kind of nice this is splitting this up. The increase in number of lines is not a bad thing in my opinion as it improves overall readability.

This comment has been minimized.

@jackgerrits

jackgerrits Jan 22, 2019

Member

@palmerlao we want to have the overflow to fill the entire line before it continues onto another, is this possible?

double wolfe_eval(vw& all, bfgs& b, float* mem, double loss_sum, double previous_loss_sum, double step_size, double importance_weight_sum, int &origin, double& wolfe1, T& weights)
template <class T>
double wolfe_eval(vw& all,
bfgs& b,

This comment has been minimized.

@JohnLangford

JohnLangford Jan 17, 2019

Collaborator

The many lines here seem a little bit unfortunate. Fixable?

This comment has been minimized.

@jackgerrits

jackgerrits Jan 18, 2019

Member

This function has heaps of parameters, I think the extra lines are okay

@@ -233,19 +234,20 @@ void finish_example(vw& all, bs& d, example& ec)
}

void finish(bs& d)
{ delete d.pred_vec; }
{

This comment has been minimized.

@JohnLangford

JohnLangford Jan 17, 2019

Collaborator

Clang seems to do small class member functions on a single line but not separate functions on a single line. This seems a little inconsistent?

This comment has been minimized.

@palmerlao

palmerlao Jan 18, 2019

Author Contributor

My understanding of BreakBeforeBraces: Allman for clang-format is that every brace has a newline before it.

For class member functions, there's a separate option that I've set as per @jackgerrits earlier comment: AllowShortFunctionsOnASingleLine: Inline.

However, I've realized that the clang-format-3.4 I installed in CI doesn't have this option, and that the script wasn't running correctly, which I'm close to fixing.

Would there be any objection to updating the Docker image so that we can get a more accessible and up-to-date version of clang-format? I'm currently on Xubuntu 16.04 and by default get clang-format 3.8, which does have the aforementioned option.

This comment has been minimized.

@JohnLangford

JohnLangford Jan 18, 2019

Collaborator

no objection here. @jackgerrits ?

This comment has been minimized.

@jackgerrits

jackgerrits Jan 18, 2019

Member

No objection at all! I recently setup auto-building for our docker image so if you could update the Dockerfile too that would be great

This comment has been minimized.

@jackgerrits

jackgerrits Jan 22, 2019

Member

So for the short functions, it's okay if they are expanded out over several lines but is it possible for it to be consistent with class functions and free functions?

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Jan 17, 2019

I took a look through. The patch seems definitely worth doing although we might want to make some tweaks around the edges. Can you take a look at the questions I had?

It's probably also worthwhile stick this:
find vowpalwabbit -type f -name '.cc' -o -name '.h' | xargs clang-format -i
into utl/new_version

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Jan 17, 2019

@jackgerrits did you have opinions on the questions I asked as well?

@palmerlao palmerlao force-pushed the palmerlao:dev/palmerlao/add-clang-format branch from 9a95ab3 to 915ac25 Jan 18, 2019

@jackgerrits

This comment has been minimized.

Copy link
Member

jackgerrits commented Jan 18, 2019

I had a couple of comments, I think the overall trend of my answers though is that I'm okay with more lines if it makes things clearer. Clearer is obviously subjective though.

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Jan 19, 2019

I guess it's clearer to me if something which is conceptually a 'line' (multiple initializers, multiple option modifiers) stays on a conceptual 'line' rather than having a discontinuous shift in format.

This does seem pretty subjective, so I think the question is: does that convince either of you? Or does anyone else care?

Unrelated, there's a build fail here that we need to fix before merging.

@palmerlao

This comment has been minimized.

Copy link
Contributor Author

palmerlao commented Jan 20, 2019

My (probably strange) opinion is that the value of the code formatter does not come from making the code more readable, but from making some style (even if it's slightly or completely different from the pre-existing style) automatically and consistently applicable.

That is, I'm okay with (and to some degree, expect) the code getting slightly uglier.

@jackgerrits

This comment has been minimized.

Copy link
Member

jackgerrits commented Jan 22, 2019

I agree with consistent and automatically applied being valuable for sure.

@jackgerrits

This comment has been minimized.

Copy link
Member

jackgerrits commented Jan 22, 2019

There are just 2 more comments and then I think we should be good to merge.

  • Overflow fill entire line rather than one item per line
  • Expand small functions consistently
@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Feb 4, 2019

Reading through clang-format docs, it looks like we want:
AllowShortFunctionsOnASingleLine: All
instead of:
AllowShortFunctionsOnASingleLine: Inline

and:
BinPackParameters: true
instead of:
BinPackParameters: false

The logic here is that the format is a little bit more consistent and concise.

Do you want to do it? (I'll merge immediately to avoid having things drag on further...)

@palmerlao

This comment has been minimized.

Copy link
Contributor Author

palmerlao commented Feb 4, 2019

I'll make some time for this tonight unless you want to try the changes you're suggesting out.
I'm not sure this is safe to merge as-is.

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Feb 4, 2019

What's the safety concern?

@palmerlao

This comment has been minimized.

Copy link
Contributor Author

palmerlao commented Feb 4, 2019

I guess if you merge master and format all the files it should be okay, but merging without re-formatting may cause the CI check to fail.

I also wanted to test the CI check more thoroughly and add the script to easily reformat everything under vowpalwabbit under utl/ like you mentioned earlier, but happy to do that in a separate PR if you're keen on getting the config in now.

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Feb 5, 2019

I see. Go ahead and take your time to get it right.

@palmerlao palmerlao force-pushed the palmerlao:dev/palmerlao/add-clang-format branch from 98fb122 to bf310b7 Feb 5, 2019

@palmerlao palmerlao force-pushed the palmerlao:dev/palmerlao/add-clang-format branch from 2cfb22f to 39c6af3 Feb 5, 2019

new_options.add(make_option("active", active_option).keep().help("enable active learning"))
.add(make_option("simulation", simulation).help("active learning simulation mode"))
.add(make_option("mellowness", data->active_c0)
.default_value(8.f)

This comment has been minimized.

@JohnLangford

JohnLangford Feb 5, 2019

Collaborator

This is still not doing what I expect :-(

How do we bin pack chained function calls?

This comment has been minimized.

@JohnLangford

JohnLangford Feb 5, 2019

Collaborator

As far as I can tell, this is not possible with clang-format.

Overall, this patch still seems worthwhile, so I'm going to merge.

@JohnLangford JohnLangford merged commit d0c9042 into VowpalWabbit:master Feb 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Feb 5, 2019

(Thank you :-) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment