search

KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
3.07k stars 842 forks source link

Add .clang-format and run formatter? #2959

Open dj2 opened 2 years ago

dj2 commented 2 years ago

Would there be any objection to adding a clang-format file and running the formatter?

I'd recommend something as simple as possible like:

# http://clang.llvm.org/docs/ClangFormatStyleOptions.html
BasedOnStyle: Chromium
greg-lunarg commented 2 years ago

One concern is the disruption to git-blame.

Is the current policy and process and code base so bad that such a disruption is necessary? Can you give an example or two that are causing you or others problems?

dj2 commented 2 years ago

Running a formatter at the end to fix up issues is definitely easier then trying to match the format of the current file. As to current policy, I don't actually know what the current format policy is? Is it written down, or is it just match whatever the current file does?

The reformat does cause you to have to step over it in git blame, but is that any different then if a file moved?

jeremy-lunarg commented 2 years ago

There is a .clang-format file in the root of the repository.

Also related: https://github.com/KhronosGroup/glslang/pull/1707

dj2 commented 2 years ago

Ah missed that. I still think it would be useful to run over all of the source as a onetime event to get everything to the same format. Otherwise, you still run into the issue that you have to match current style.

greg-lunarg commented 2 years ago

Ultimately, yes, I think we should toward this practice.

Before you go to the trouble of submitting a PR, I would like to review the current .clang-format and the impact it would have.

dj2 commented 2 years ago

It would be nice if we switched to one of the standard formats so the clang-format is just a BasedOn: Chromium line or something similar.

greg-lunarg commented 2 years ago

It would be nice if we switched to one of the standard formats so the clang-format is just a BasedOn: Chromium line or something similar.

I will look at that.

jeremy-lunarg commented 2 years ago

If we're going to do this, I think we should have a check in CI that enforces it. Otherwise, we'll just end up here again.

karl-lunarg commented 2 years ago

I don't have any personal experience with this, but git blame has a --ignore-rev <sha1> option. You can also place a .git-blame-ignore-revs file in the repo that contains revisions that are formatting-only. You still need to say git blame --ignore-revs-file .git-blame-ignore-revs, but one can set a git config option blame.ignoreRevsFile to take care of that.