clang-format: Set InsertNewlineAtEOF: true #33896

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2511-clang-format-eof-newline changing 1 files +1 −1
  1. maflcko commented at 7:49 am on November 18, 2025: member

    Now that the minimum supported clang version is 17, the InsertNewlineAtEOF setting can be set to true in the clang-format file. (https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#insertnewlineateof)

    This is in line with the already existing newline linter. Can be tested via:

    0truncate --size=-1 src/init.cpp
    1git diff
    2
    3# Should fail:
    4cargo run --manifest-path ./test/lint/test_runner/Cargo.toml -- --lint=trailing_newline
    5
    6# Restore newline:
    7git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
    
  2. clang-format: Set InsertNewlineAtEOF: true fa1bf6818f
  3. DrahtBot commented at 7:49 am on November 18, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33896.

    Reviews

    See the guideline for information on the review process.

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  4. maflcko added the label Refactoring on Nov 18, 2025
  5. hodlinator approved
  6. hodlinator commented at 9:48 am on November 18, 2025: contributor

    ACK fa1bf6818f0910f997e5235a197ff51f2e18780d

    Makes sense for this project to conform to the UNIXy style of ending all lines with \n.

    Previous setting of having it false comes from 13f36c020f0329b5e975282b45292fdf2a495e31 which just added a bunch of newer settings to src/.clang-format with default clang-format values, rather than being a conscious decision by this project.

    Have recently been discussing how GitHub and Git also seem to agree on this: #28792 (review)

  7. in src/.clang-format:126 in fa1bf6818f
    122@@ -123,7 +123,7 @@ IndentRequiresClause: true
    123 IndentWidth:     4
    124 IndentWrappedFunctionNames: false
    125 InsertBraces:    false
    126-InsertNewlineAtEOF: false
    127+InsertNewlineAtEOF: true
    


    l0rinc commented at 10:01 am on November 18, 2025:

    llvm@17 has a KeepEmptyLinesAtEOF: false as well - added exactly to avoid the problem we’re trying to avoid here: https://github.com/llvm/llvm-project/issues/56054 Should we make it explicit here?

     0diff --git a/src/.clang-format b/src/.clang-format
     1--- a/src/.clang-format	(revision 9d5bad11b6cbf50c776f9e9a583ac1fb79271fe8)
     2+++ b/src/.clang-format	(date 1763460129408)
     3@@ -123,7 +123,7 @@
     4 IndentWidth:     4
     5 IndentWrappedFunctionNames: false
     6 InsertBraces:    false
     7-InsertNewlineAtEOF: false
     8+InsertNewlineAtEOF: true
     9 InsertTrailingCommas: None
    10 IntegerLiteralSeparator:
    11   Binary:          0
    12@@ -135,6 +135,7 @@
    13 JavaScriptQuotes: Leave
    14 JavaScriptWrapImports: true
    15 KeepEmptyLinesAtTheStartOfBlocks: false
    16+KeepEmptyLinesAtEOF: true
    17 LambdaBodyIndentation: Signature
    18 LineEnding:      DeriveLF
    19 MacroBlockBegin: ''
    

    Note that these were replaced again in llvm@19: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#keepemptylines


    maflcko commented at 10:24 am on November 18, 2025:

    llvm@17 has a KeepEmptyLinesAtEOF: false as well - added exactly to avoid the problem we’re trying to avoid here: llvm/llvm-project#56054 Should we make it explicit here?

    Not sure I understand the problem you are referring to. InsertNewlineAtEOF is already set to true in this pull request. Can you add exact steps to reproduce the problem?


    l0rinc commented at 10:31 am on November 18, 2025:

    InsertNewlineAtEOF is already set to true in this pull

    Yes, please scroll to the bottom of the patch above, I’m also setting KeepEmptyLinesAtEOF there.

    My understanding from https://github.com/llvm/llvm-project/issues/63150 is that InsertNewlineAtEOF alone doesn’t work in some versions

    The clang format option InsertNewlineAtEOF : true is not being applied, instead the newline is being removed

    , so they have added KeepEmptyLinesAtEOF in 17 https://reviews.llvm.org/D152305:

    Adds an option KeepEmptyLinesAtEOF to keep empty lines (up to MaxEmptyLinesToKeep) before EOF. This remedies the probably unintentional change in behavior introduced in rG3d3ea8, which started to always remove empty lines before EOF.

    As mentioned, it’s a bit hacky, since they have deprecated KeepEmptyLinesAtEOF in 19 - but we have some time until we upgrade to 19, we should probably temporarily set it


    hodlinator commented at 10:38 am on November 18, 2025:

    As I understand it some people wanted to keep additional empty lines at the end of the file (\n\n). https://github.com/llvm/llvm-project/issues/63150#issuecomment-1579150950

    I don’t think this is a concern for us?


    l0rinc commented at 10:43 am on November 18, 2025:

    The issue claims:

    For C and C++ an empty line at the end of each file (header and implementation) is mandatory.

    Hmmm, so maybe the problem was that multiple empty lines were changes to none? I can try it out a bit later, but I’m working on something else locally…


    maflcko commented at 11:16 am on November 18, 2025:

    I don’t think this is a concern for us?

    Agree, I don’t see the issue for us, but happy to test, if there are steps to test.

    Hmmm, so maybe the problem was that multiple empty lines were changes to none?

    I can’t reproduce this locally:

    0echo '' >> src/init.cpp
    1echo '' >> src/init.cpp
    2git diff
    3
    4# Restore newline:
    5git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
    

    I’ll leave this as-is for now, but I am happy to review a separate pull changing the value of a separate setting, if there is need to.


    l0rinc commented at 12:40 pm on November 18, 2025:
    Thanks for checking
  8. stickies-v approved
  9. stickies-v commented at 11:34 am on November 18, 2025: contributor
    ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
  10. hebasto approved
  11. hebasto commented at 11:40 am on November 18, 2025: member
    ACK fa1bf6818f0910f997e5235a197ff51f2e18780d.
  12. l0rinc commented at 12:41 pm on November 18, 2025: contributor
    ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
  13. janb84 commented at 12:47 pm on November 18, 2025: contributor

    ACK fa1bf6818f0910f997e5235a197ff51f2e18780d

    PR unifies the enforcement of the new line at end of file to also include the rule in the clang-format rules (is already in the linter and in the .editorconfig)

    Tested and works as advertised + learned of the existence a new script in contrib :)

  14. achow101 commented at 0:17 am on November 20, 2025: member
    ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
  15. achow101 merged this on Nov 20, 2025
  16. achow101 closed this on Nov 20, 2025

  17. maflcko deleted the branch on Nov 20, 2025

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-11-23 00:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me