Add explicit values to .clang-format

[sconvent]

Summary

Results differed on different systems without explicitly setting the added arguments

[jlpeyton]

runtime/.clang-format

6

This disables clang-format altogether when using -style=file? Why would we want this and doesn't it make other changes to this file useless? I believe we should just leave this file alone and clang-format the OMPT changes based on what the current configuration is.

[protze.joachim]

runtime/.clang-format

6

I understand the comments below as the expected values for

clang-format -dump-config -style=file

What we added is the diff of our output with the commented output. If the value is supposed to be false (as it is by default on my system/version) then this should also be changed below; and the value should still be explicitly listed as on some system the default seems to be true.

[protze.joachim]

For current HEAD of master I get a different result for any combination of the added values (unless I add the DisableFormat=true).
The smallest diff I get if I only add

MaxEmptyLinesToKeep: 2

[Hahnfeld]

So maybe the reason for that is using different versions of clang-format? @jlpeyton can you share which version you have been using to initially format the source code?

[AndreyChurbanov]

In D38837#896170, @Hahnfeld wrote:

So maybe the reason for that is using different versions of clang-format? @jlpeyton can you share which version you have been using to initially format the source code?

Right. We found that each (we haven't tried all versions of cause:) version of the clang-format produces different formatting, so we decided to stick with the version 3.8.1:
$ clang-format --version
clang-format version 3.8.1 (http://llvm.org/git/clang.git 9fd77bd68130d9b2fbc56a3138b6f981d560480a) (http://llvm.org/git/llvm.git 8b47c17a53d683f313eaaa93c4a53de26d8fcba5)

[Hahnfeld]

In D38837#896173, @AndreyChurbanov wrote:

Right. We found that each (we haven't tried all versions of cause:) version of the clang-format produces different formatting, so we decided to stick with the version 3.8.1:
$ clang-format --version
clang-format version 3.8.1 (http://llvm.org/git/clang.git 9fd77bd68130d9b2fbc56a3138b6f981d560480a) (http://llvm.org/git/llvm.git 8b47c17a53d683f313eaaa93c4a53de26d8fcba5)

So maybe some if this is needed to get it also to work with newer versions because they added options?

[jlpeyton]

That is peculiar that the DisableFormat attribute is true for our defaults in comments. I don't know why that is.

Anyways, I thought one could use git clang-format to only format the OMPT changes, or any current change. You do not have to format the entire file or maybe I have this wrong?

What I would try (if using git) is

1. Remove these changes to .clang-format
2. Take the large OMPT patch, create a separate branch based on current master
3. Apply the patch, and commit only the src/* files there. It looks like the test files shouldn't be run through clang-format because they contain large // CHECK: lines which should not be broken up. You can pick and choose of course.
4. Run:

$ git clang-format HEAD~1
This should reformat only the changes different between HEAD~1..HEAD (the large OMPT patch) and leave the reformatting changes in unstaged files so they are easy to get rid of if something goes wrong.

5. Commit the reformatting changes
6. Mash those new formatting changes in with the current OMPT patch
7. Submit it again to Phabricator :-D

The OMPT patch currently has lines that go over 80 characters (e.g., kmp_tasking.cpp) suggesting that the current OMPT patch has not been run through any real clang-format filter.

[Hahnfeld]

This might work for one patch but still doesn't solve the future problem, i.e. we don't want to be bound to 3.8.1 forever.

(I think Joachim came across this because he indeed had to format all sources: The changes were based on a commit before the code got reformatted. So if I remember correctly, he just formatted everything again so that the number of merge conflicts was reduced)

[jlpeyton]

Ah ok, I see now. When we first created .clang-format some specific attributes were changed from the defaults and not properly added. So we just need to remove the DisableFormat line, change the DisableFormat line in the comments to false (as Joachim noted), and the rest can stay. The OMPT patch still needs to be run through clang-format in some manner since the DisableFormat line probably prevented any real formatting to occur. After those changes this one looks good to me.

[protze.joachim]

I tried to update the attached diff, but accidently created a new differential: https://reviews.llvm.org/D38920
I built a clang-format using the source suggested by AndreyChurbanov. I still get a diff for the source of master.

[jlpeyton]

Ok, I've done some more research on this and here is what I've found and suggest as the solution:

1. It appears we didn't clang-format our codebase to completion or somehow these changes in https://reviews.llvm.org/D38920 slipped through the cracks. I don't know. Whatever the case, it is a good idea to commit these remaining format changes.
2. The current non-commented part of .clang-format is fine and does not need changes to it. Not even the MaxEmptyLinesToKeep change. The changes that occur when MaxEmptyLinesToKeep=1 is tidying up around the LLVM license inclusion parts in the comment headers in each file. Those changes are fine and would be consistent with our internal code base. That being said, the commented parts are confusing and should be removed as they are not consistent with the defaults for clang-format.
3. Different versions of clang-format can produce different results when you clang-format an entire file and there is no great solution. I've tried 3.7.1, 3.8.1, 3.9.1, 4.0 and all have given slightly different formatting for different parts of the code. If you try to include a new Attribute: Value pair in the .clang-format file and use an old clang-format executable, then the .clang-format file is ignored and the "fallback-style" is used (defaults to LLVM). For example, if you use an attribute added in v3.9, then you can only use v3.9+. Also, from what I've seen, the defaults do not change from 3.7.1 to 4.0.

A compromise to 3) is to use git clang-format which I've described above which only formats the changes in your commit and does not format the entire file. This way formatting is restricted to your changes and will not affect unrelated code. If all users tried to do the clang-format *.cpp *.h lines for every commit, and they are using differing clang-format versions, we would get oscillating formatting changes to unrelated parts of the code.

So the next steps I would suggest are this:

1. Remove all changes to non-commented part of .clang-format, Remove all comments in .clang-format.
2. Run clang-format 3.8.1 on the current codebase to incorporate the missing formatting changes in https://reviews.llvm.org/D38920 and also new changes removing double newlines.
3. Do something like I've described above on the OMPT change. It still needs to be run through clang-format and upload that to Phabricator.

Joachim, if you are ok with this I can do 1) and 2) and commit. Is this ok?

[protze.joachim]

In D38837#899143, @jlpeyton wrote:

Ok, I've done some more research on this and here is what I've found and suggest as the solution:

1. It appears we didn't clang-format our codebase to completion or somehow these changes in https://reviews.llvm.org/D38920 slipped through the cracks. I don't know. Whatever the case, it is a good idea to commit these remaining format changes.

I updated the attached diff in https://reviews.llvm.org/D38920 with the old format file. Is runtime/src/*.cpp runtime/src/*.h sufficient, or should other files be formated too?

2. The current non-commented part of .clang-format is fine and does not need changes to it. Not even the MaxEmptyLinesToKeep change. The changes that occur when MaxEmptyLinesToKeep=1 is tidying up around the LLVM license inclusion parts in the comment headers in each file. Those changes are fine and would be consistent with our internal code base. That being said, the commented parts are confusing and should be removed as they are not consistent with the defaults for clang-format.

I removed them in https://reviews.llvm.org/D38920. Simon can update this differential at the end of this week.

3. Different versions of clang-format can produce different results when you clang-format an entire file and there is no great solution. I've tried 3.7.1, 3.8.1, 3.9.1, 4.0 and all have given slightly different formatting for different parts of the code. If you try to include a new Attribute: Value pair in the .clang-format file and use an old clang-format executable, then the .clang-format file is ignored and the "fallback-style" is used (defaults to LLVM). For example, if you use an attribute added in v3.9, then you can only use v3.9+. Also, from what I've seen, the defaults do not change from 3.7.1 to 4.0.

A compromise to 3) is to use git clang-format which I've described above which only formats the changes in your commit and does not format the entire file. This way formatting is restricted to your changes and will not affect unrelated code. If all users tried to do the clang-format *.cpp *.h lines for every commit, and they are using differing clang-format versions, we would get oscillating formatting changes to unrelated parts of the code.

The main question remaining is, to what files should we apply clang-format. You already pointed out that we should not format tests.

So the next steps I would suggest are this:

1. Remove all changes to non-commented part of .clang-format, Remove all comments in .clang-format.
2. Run clang-format 3.8.1 on the current codebase to incorporate the missing formatting changes in https://reviews.llvm.org/D38920 and also new changes removing double newlines.
3. Do something like I've described above on the OMPT change. It still needs to be run through clang-format and upload that to Phabricator.

For the diff, that I uploaded on the weekend, I already applied clang-format - with the same values as in https://reviews.llvm.org/D38920.

These are the changes for using the old clang-format file:
https://reviews.llvm.org/D38185?vs=119034&id=119252&whitespace=ignore-most#toc

Joachim, if you are ok with this I can do 1) and 2) and commit. Is this ok?

Ok!

[jlpeyton]

The main question remaining is, to what files should we apply clang-format. You already pointed out that we should not format tests.

We also need to include kmp_itt.inl. So *.cpp *.h *.inl should do the trick.

runtime/.clang-format

 ---
 BasedOnStyle:  LLVM
 AlignTrailingComments: false
 SortIncludes:    false
+AlignOperands: false
+DisableFormat:   true
+KeepEmptyLinesAtTheStartOfBlocks: false
+MaxEmptyLinesToKeep: 2
 ---
 # Language: Cpp
 # AccessModifierOffset: -2
 # AlignAfterOpenBracket: Align
 # AlignConsecutiveAssignments: false
 # AlignConsecutiveDeclarations: false
 # AlignEscapedNewlinesLeft: false
 # AlignOperands: false