[Openmp-commits] [PATCH] D38837: Add explicit values to .clang-format

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added a comment.

In https://reviews.llvm.org/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!


https://reviews.llvm.org/D38837