[clang-format][NFC] Set LineEnding to LF in config files

[owenpan]

Summary

To prevent \r\n line endings from getting into the source files.

I'd rather change the default to false for LLVM style though. What do you think?

[rymiel]

The LLVM Coding Standard apparently doesn't mention line endings..? A quick grep does show a bunch of \r\n results, primarily in tests. I'd be in favor of forcing \n, but I think this is a wider matter..?

[owenpan]

In D141098#4030426, @rymiel wrote:

The LLVM Coding Standard apparently doesn't mention line endings..?

Line endings probably should never be specified in coding standards, but the default should match the majority, and in the case of LLVM style, it should be \n IMO.

A quick grep does show a bunch of \r\n results, primarily in tests.

It's possible that some/most of those \r\n get in by accident like in https://github.com/llvm/llvm-project/blob/617277e7cbdaea6881425c7a1a5b1cf4b1d4b507/clang/unittests/Format/FormatTest.cpp? And for tests, clang-format can be disabled like in D128706.

I think we should combine DeriveLineEnding and UseCRLF, with the default being LF for LLVM. If you all agree, I will abandon this patch and implement a new option and deprecate the current ones.

[rymiel]

I think we should combine DeriveLineEnding and UseCRLF

Do you mean a single LineEnding options which is an enum with LF, CRLF and Derive, or similar?

[owenpan]

In D141098#4032951, @rymiel wrote:

I think we should combine DeriveLineEnding and UseCRLF

Do you mean a single LineEnding options which is an enum with LF, CRLF and Derive, or similar?

Yep.

[MyDeveloperDay]

I kind of agree, it would be nice not to have to use dos2unix and it be set by default.

[owenpan]

Unfortunately, changing the default value of DeriveLineEnding to false would break too many unit tests.

[owenpan]

I will change DeriveLineEnding: false to LineEnding: LF after D141654 lands.

[sunho]

This seems to be causing builtbot lint script error: https://buildkite.com/llvm-project/premerge-checks/builds/134501#01862630-8fa9-487c-99b1-9addbe6257d0

[MyDeveloperDay]

I guess we'll need to revert this until the buildbots are updated.

[owenpan]

Why don't we fix the buildbot instead? Why does it use a pre-16 version to test 16 source?

[sunho]

I think buildbot is "formatting" the patch using clang-format-15. The tests of clang-format would be done with latest code.

[owenpan]

In D141098#4108286, @sunho wrote:

I think buildbot is "formatting" the patch using clang-format-15. The tests of clang-format would be done with latest code.

Can we just fix the buildbot so that it runs git-clang-format with --binary? Using clang-format-15 to format a patch to clang-format-16 source still looks wrong to me.

[sunho]

Wouldn't that mean llvm contributors have to format their code using clang-format built from top-of-tree source code before submitting the patch? Technically that script is for checking if the code has been formatted correctly.

[owenpan]

In D141098#4111830, @sunho wrote:

Wouldn't that mean llvm contributors have to format their code using clang-format built from top-of-tree source code before submitting the patch?

Only if the patch is to clang-format source.

[sunho]

I think it makes more sense to use stable version of clang-format for that purpose but I'm not in a postion to decide as I don't work with format. Can you change the script to use latest clang-format for format codebase and revert this patch in the meantime? Currently, all patches that touch format part will see format verifiaction ci faliure with this patch.

[owenpan]

In D141098#4111890, @sunho wrote:

I think it makes more sense to use stable version of clang-format for that purpose but I'm not in a postion to decide as I don't work with format. Can you change the script to use latest clang-format for format codebase and revert this patch in the meantime?

We don't want to change git-clang-format on a whim. If you think there is an issue, please file a bug report.

Currently, all patches that touch format part will see format verifiaction ci faliure with this patch.

That's not true. We've had several clang-format commits since this patch landed. We don't want to defeat the purpose (or delay the effect) of this patch and should let the premerge check fail if the patch to the current format source is not formatted by the latest clang-format.

This is the only buildbot reported to be impacted by this patch. Can we fix the bot instead? In addition to using --binary of git-clang-format as I suggested before, another way would be to format a patch to files outside the format directory?

[mehdi_amini]

In D141098#4111809, @owenpan wrote:

Can we just fix the buildbot so that it runs git-clang-format with --binary? Using clang-format-15 to format a patch to clang-format-16 source still looks wrong to me.

The general principle is that developers of LLVM 16 should be able to use clang-format from the previous release (that is: one where we have prebuilt binaries available) to format patches in general.
It does not seem reasonable to me that we should need to build clang-format ourselves for the purpose of writing patches to LLVM.

[owenpan]

In D141098#4192785, @mehdi_amini wrote:

In D141098#4111809, @owenpan wrote:

Can we just fix the buildbot so that it runs git-clang-format with --binary? Using clang-format-15 to format a patch to clang-format-16 source still looks wrong to me.

The general principle is that developers of LLVM 16 should be able to use clang-format from the previous release (that is: one where we have prebuilt binaries available) to format patches in general.
It does not seem reasonable to me that we should need to build clang-format ourselves for the purpose of writing patches to LLVM.

Agreed, except for the clang-format source, which its .clang-format files are part of.

clang/include/clang/Format/.clang-format

 BasedOnStyle: LLVM
 InsertBraces: true
+LineEnding: LF
 RemoveBracesLLVM: true

clang/lib/Format/.clang-format

 BasedOnStyle: LLVM
 InsertBraces: true
+LineEnding: LF
 RemoveBracesLLVM: true

clang/unittests/Format/.clang-format

 BasedOnStyle: LLVM
 InsertBraces: true
+LineEnding: LF
 RemoveBracesLLVM: true