[clang-format] Add IgnoreExtension to SortIncludes

[DaanDeMeyer]

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 an 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 is arguably 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 adding IgnoreExtension
to SortIncludes.

[llvmbot]

@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.

[DaanDeMeyer]

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.

[HazardyKnusperkeks]

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

[HazardyKnusperkeks]

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]

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

[DaanDeMeyer]

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

[github-actions]

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

[DaanDeMeyer]

Ping @owenca @HazardyKnusperkeks

[owenca]

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]

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

[DaanDeMeyer]

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

[mydeveloperday]

Do we have the correct current default

[DaanDeMeyer]

Ping? Would love to get this over the finish line

[owenca]

Please fix all compiler warnings, e.g.

llvm-project/clang/lib/Format/Format.cpp:667:70: warning: missing field 'IgnoreExtension' initializer [-Wmissing-field-initializers]
  667 |                                                   /*IgnoreCase=*/true}));
      |                                                                      ^

Also, the new option is not recognized:

$ cat .clang-format
SortIncludes:
  Enabled: true
  IgnoreExtension: true
$ clang-format -dump-config
.clang-format:3:3: error: unknown key 'IgnoreExtension'
  IgnoreExtension: true
  ^~~~~~~~~~~~~~~

[DaanDeMeyer]

@owenca @mydeveloperday @HazardyKnusperkeks Can any of you merge this? I don't have commit rights