clang-format: Add IncludeSortKey option #137840

DaanDeMeyer · 2025-04-29T16:29:47Z

Sorting by stem gives nicer results when various header file names
are substrings of other header file names, for example, a CLI application
with a main header named analyze.h and a analyze-xxx.h header for each
subcommand currently will always put analyze.h last after all the
analyze-xxx.h headers, but putting analyze.h first instead of last is arguable
nicer to read.

TLDR; Instead of

#include "analyze-blame.h"
#include "analyze.h"

You'd get

#include "analyze.h"
#include "analyze-blame.h"

Let's allow sorting by stem instead of full path by introducing a new
option IncludeSortKey with two values "Path" and "Stem".

llvmbot · 2025-04-29T16:30:23Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: Daan De Meyer (DaanDeMeyer)

Changes

Sorting by stem gives nicer results when various header file names are substrings of other header file names, for example, a CLI application with a main header named analyze.h and a analyze-xxx.h header for each subcommand currently will always put analyze.h last after all the analyze-xxx.h headers, but putting analyze.h first instead of last is arguable nicer to read.

TLDR; Instead of

#include "analyze-blame.h"
#include "analyze.h"

We'll now get

#include "analyze.h"
#include "analyze-blame.h"

Full diff: https://github.com/llvm/llvm-project/pull/137840.diff

1 Files Affected:

(modified) clang/lib/Format/Format.cpp (+10-8)

diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 5a1c3f556b331..bc1e681c9be78 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -3219,17 +3219,19 @@ static void sortCppIncludes(const FormatStyle &Style,
 
   if (Style.SortIncludes == FormatStyle::SI_CaseInsensitive) {
     stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
-      const auto LHSFilenameLower = Includes[LHSI].Filename.lower();
-      const auto RHSFilenameLower = Includes[RHSI].Filename.lower();
-      return std::tie(Includes[LHSI].Priority, LHSFilenameLower,
-                      Includes[LHSI].Filename) <
-             std::tie(Includes[RHSI].Priority, RHSFilenameLower,
-                      Includes[RHSI].Filename);
+      const auto LHSStem = llvm::sys::path::stem(Includes[LHSI].Filename);
+      const auto RHSStem = llvm::sys::path::stem(Includes[RHSI].Filename);
+      const auto LHSStemLower = LHSStem.lower();
+      const auto RHSStemLower = RHSStem.lower();
+      return std::tie(Includes[LHSI].Priority, LHSStemLower, LHSStem) <
+             std::tie(Includes[RHSI].Priority, RHSStemLower, RHSStem);
     });
   } else {
     stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
-      return std::tie(Includes[LHSI].Priority, Includes[LHSI].Filename) <
-             std::tie(Includes[RHSI].Priority, Includes[RHSI].Filename);
+      const auto LHSStem = llvm::sys::path::stem(Includes[LHSI].Filename);
+      const auto RHSStem = llvm::sys::path::stem(Includes[RHSI].Filename);
+      return std::tie(Includes[LHSI].Priority, LHSStem) <
+             std::tie(Includes[RHSI].Priority, RHSStem);
     });
   }

HazardyKnusperkeks

You definitely need some tests. And I'm not sure if everyone would like that, of if it should be an option.

clang/lib/Format/Format.cpp

DaanDeMeyer · 2025-04-29T18:57:08Z

You definitely need some tests. And I'm not sure if everyone would like that, of if it should be an option.

I was hoping to get some feedback on whether it should be an option or not before adding tests.

This was done by running a locally built clang-format with llvm/llvm-project#137617 and llvm/llvm-project#137840 applied on all .c and .h files.

HazardyKnusperkeks

How do you think should files be handled, that only differ in the caseness of the extension?

HazardyKnusperkeks · 2025-04-30T19:36:38Z

You definitely need some tests. And I'm not sure if everyone would like that, of if it should be an option.

I was hoping to get some feedback on whether it should be an option or not before adding tests.

I would be in favor.

owenca · 2025-05-01T07:11:11Z

IMO, unless it's a bug, we need a new option to ensure it's a non-breaking change.

DaanDeMeyer · 2025-05-01T11:55:20Z

@HazardyKnusperkeks @owenca Updated PR to introduce a new option IncludeSortKey, PTAL!

github-actions · 2025-05-01T11:56:25Z

✅ With the latest revision this PR passed the C/C++ code formatter.

clang/include/clang/Tooling/Inclusions/IncludeStyle.h

clang/docs/ClangFormatStyleOptions.rst

clang/include/clang/Tooling/Inclusions/IncludeStyle.h

DaanDeMeyer · 2025-05-12T17:53:47Z

Ping @owenca @HazardyKnusperkeks

owenca · 2025-05-13T07:28:27Z

I would expect that file extensions be sorted regardless. For example:

#include "a.h"
#include "a.inc"
#include "a-util.def"
#include "a-util.h"

DaanDeMeyer · 2025-05-13T07:52:32Z

I would expect that file extensions be sorted regardless. For example:
#include "a.h"
#include "a.inc"
#include "a-util.def"
#include "a-util.h"

Addressed

clang/docs/ClangFormatStyleOptions.rst

clang/lib/Format/Format.cpp

DaanDeMeyer · 2025-05-15T14:35:38Z

@owenca Thanks for the detailed review. Changed the name to IncludeSortIgnoreExtension, let me know if you have a better idea for the name.

This allows adding other suboptions e.g. IgnoreExtension in llvm#137840.

clang/include/clang/Format/Format.h

clang/include/clang/Tooling/Inclusions/IncludeStyle.h

This allows adding other suboptions e.g. IgnoreExtension in #137840.

DaanDeMeyer · 2025-05-19T10:44:10Z

@owenca @HazardyKnusperkeks Rebased onto latest main to take advantage of @owenca's rework and addressed comments.

mydeveloperday

Do we have the correct current default

mydeveloperday · 2025-05-19T15:39:54Z

clang/lib/Format/Format.cpp

@@ -1647,7 +1647,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
  LLVMStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Leave;
  LLVMStyle.ShortNamespaceLines = 1;
  LLVMStyle.SkipMacroDefinitionBody = false;
-  LLVMStyle.SortIncludes = {/*Enabled=*/true, /*IgnoreCase=*/false};
+  LLVMStyle.SortIncludes = {/*Enabled=*/true, /*IgnoreCase=*/false, /*IgnoreExtension=*/false};


if I understand correctly... true is the current order not false, so the default should be true otherwise you'll cause every user of clang-format to have to add the IgnoreExtension setting if they don't want a change, am I correct?

@mydeveloperday The default is false, by default, the extension is not ignored.

in the documentation you show, neither true/false gives you what clang-format gives us today (with just BasedOnStyle: LLVM

which is ...

#include "A.h" #include "A-util.h" #include "A.inc"

So lets assume I don't what your feature... how do I keep my current format? what should it be set to true or false?

true: false: # include "A.h" # include "A-util.h" # include "A.inc" # include "A.h" # include "A-util.h" # include "A.inc"

"currently will always put analyze.h last after all the analyze-xxx.h headers, but putting analyze.h first instead of last is arguable nicer to read"

How come for me its doesn't do this as you say...

seems your example might not be the best for the documentation, seems like this is dependent on the basename length being greater than 1 in length. Ok so thats confusing...the documentation should represent what happens.

#include "ab-beta.h" #include "ab-data.h" #include "ab.h"

#include "a.h" #include "a-beta.h" #include "a-data.h"

@mydeveloperday Huh, I don't see how the length of the basename would affect this. I pushed more tests to verify this is the case.

#include "A.h" #include "A-util.h" #include "A.inc"

I don't see how clang-format would ever return this today without disabling include sorting? - sorts before . so A-util.h will always be put before A.h and A.inc today if include sorting is enabled.

Is the file maybe A.cpp?

yes.. thats it.. it was A.cxx actually..

I'm very confused now what the suggestion is that I should apply here to make everyone happy

Sorting without taking the file extension into account gives nicer results when various header file names are substrings of other header file names, for example, a CLI application with a main header named analyze.h and a analyze-xxx.h header for each subcommand currently will always put analyze.h last after all the analyze-xxx.h headers, but putting analyze.h first instead of last is arguable nicer to read. TLDR; Instead of """ /#include "analyze-blame.h" /#include "analyze.h" """ You'd get """ /#include "analyze.h" /#include "analyze-blame.h" """ Let's allow sorting without taking the file extension into account unless two headers otherwise compare equal by introducing a new boolean option IgnoreExtension for SortIncludesOptions.

llvmbot added the clang-format label Apr 29, 2025

HazardyKnusperkeks reviewed Apr 29, 2025

View reviewed changes

clang/lib/Format/Format.cpp Outdated Show resolved Hide resolved

DaanDeMeyer added a commit to DaanDeMeyer/systemd that referenced this pull request Apr 29, 2025

tree-wide: Sort includes

7f10785

This was done by running a locally built clang-format with llvm/llvm-project#137617 and llvm/llvm-project#137840 applied on all .c and .h files.

DaanDeMeyer mentioned this pull request Apr 29, 2025

Add our own <netinet/in.h> and <net/if.h> headers and sort includes tree-wide with clang-format systemd/systemd#37278

Merged

DaanDeMeyer added a commit to DaanDeMeyer/systemd that referenced this pull request Apr 29, 2025

tree-wide: Sort includes

bcea8be

This was done by running a locally built clang-format with llvm/llvm-project#137617 and llvm/llvm-project#137840 applied on all .c and .h files.

DaanDeMeyer force-pushed the stem branch from 33b26cd to 4747e31 Compare April 29, 2025 20:19

DaanDeMeyer added a commit to DaanDeMeyer/systemd that referenced this pull request Apr 29, 2025

tree-wide: Sort includes

e8cb359

This was done by running a locally built clang-format with llvm/llvm-project#137617 and llvm/llvm-project#137840 applied on all .c and .h files.

DaanDeMeyer force-pushed the stem branch from 4747e31 to ce0cb85 Compare April 29, 2025 21:04

DaanDeMeyer added a commit to DaanDeMeyer/systemd that referenced this pull request Apr 29, 2025

tree-wide: Sort includes

8b52f06

This was done by running a locally built clang-format with llvm/llvm-project#137617 and llvm/llvm-project#137840 applied on all .c and .h files.

DaanDeMeyer added a commit to DaanDeMeyer/systemd that referenced this pull request Apr 30, 2025

tree-wide: Sort includes

a9532b0

This was done by running a locally built clang-format with llvm/llvm-project#137617 and llvm/llvm-project#137840 applied on all .c and .h files.

DaanDeMeyer added a commit to DaanDeMeyer/systemd that referenced this pull request Apr 30, 2025

tree-wide: Sort includes

1cf4069

This was done by running a locally built clang-format with llvm/llvm-project#137617 and llvm/llvm-project#137840 applied on all .c and .h files.

HazardyKnusperkeks reviewed Apr 30, 2025

View reviewed changes

HazardyKnusperkeks requested review from owenca and mydeveloperday April 30, 2025 19:36

DaanDeMeyer force-pushed the stem branch from ce0cb85 to 4edcb6e Compare May 1, 2025 11:53

llvmbot added the clang Clang issues not falling into any other category label May 1, 2025

DaanDeMeyer changed the title ~~clang-format: Sort includes by stem rather than full filename~~ clang-format: Add IncludeSortKey option May 1, 2025

DaanDeMeyer force-pushed the stem branch from 4edcb6e to 73a18a0 Compare May 1, 2025 11:58

HazardyKnusperkeks reviewed May 1, 2025

View reviewed changes

DaanDeMeyer force-pushed the stem branch from 73a18a0 to 7e2463f Compare May 2, 2025 13:20

DaanDeMeyer force-pushed the stem branch from 7e2463f to 20939dd Compare May 13, 2025 07:49

owenca reviewed May 14, 2025

View reviewed changes

clang/docs/ClangFormatStyleOptions.rst Outdated Show resolved Hide resolved

clang/lib/Format/Format.cpp Outdated Show resolved Hide resolved

clang/lib/Format/Format.cpp Outdated Show resolved Hide resolved

DaanDeMeyer force-pushed the stem branch from 20939dd to 672f179 Compare May 15, 2025 14:35

owenca added a commit to owenca/llvm-project that referenced this pull request May 19, 2025

[clang-format][NFC] Upgrade SortIncludes option to a struct

186ac09

This allows adding other suboptions e.g. IgnoreExtension in llvm#137840.

owenca mentioned this pull request May 19, 2025

[clang-format][NFC] Upgrade SortIncludes option to a struct #140497

Merged

HazardyKnusperkeks reviewed May 19, 2025

View reviewed changes

clang/include/clang/Format/Format.h Outdated Show resolved Hide resolved

clang/include/clang/Tooling/Inclusions/IncludeStyle.h Outdated Show resolved Hide resolved

owenca added a commit that referenced this pull request May 19, 2025

[clang-format][NFC] Upgrade SortIncludes option to a struct (#140497)

5ddcd76

This allows adding other suboptions e.g. IgnoreExtension in #137840.

DaanDeMeyer force-pushed the stem branch from 672f179 to ab2206d Compare May 19, 2025 10:43

DaanDeMeyer force-pushed the stem branch 2 times, most recently from 33f84b2 to beab0ec Compare May 19, 2025 11:44

mydeveloperday requested changes May 19, 2025

View reviewed changes

DaanDeMeyer force-pushed the stem branch 2 times, most recently from bb5eb38 to 6a3e7f0 Compare May 19, 2025 20:47

DaanDeMeyer force-pushed the stem branch from 6a3e7f0 to 3cf3ada Compare May 20, 2025 15:54

clang-format: Add IncludeSortKey option #137840

Are you sure you want to change the base?

clang-format: Add IncludeSortKey option #137840

Uh oh!

Conversation

DaanDeMeyer commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HazardyKnusperkeks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaanDeMeyer commented Apr 29, 2025

Uh oh!

HazardyKnusperkeks left a comment

Choose a reason for hiding this comment

Uh oh!

HazardyKnusperkeks commented Apr 30, 2025

Uh oh!

owenca commented May 1, 2025

Uh oh!

DaanDeMeyer commented May 1, 2025

Uh oh!

github-actions bot commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaanDeMeyer commented May 12, 2025

Uh oh!

owenca commented May 13, 2025

Uh oh!

DaanDeMeyer commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaanDeMeyer commented May 15, 2025

Uh oh!

Uh oh!

Uh oh!

DaanDeMeyer commented May 19, 2025

Uh oh!

mydeveloperday left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaanDeMeyer commented Apr 29, 2025 •

edited

Loading

llvmbot commented Apr 29, 2025 •

edited

Loading

github-actions bot commented May 1, 2025 •

edited

Loading