[clang-format] bad formatting suggestion

[Bigcheese]

The formatting suggestion here is wrong: #119740 (comment)

It's basically suggesting:

namespace N {
class C {
  // ...
  };
  } // namespace N
  void topLevelDecl() {}

This should be reproducible with:
git-clang-format --diff 4b825c7417f72ee88ee3e4316d0c01ed463f1241 11faee533d07aff6955d44bda0d502888e52e119 --extensions cpp,c,h -- clang/include/clang/Lex/ModuleMapFile.h clang/lib/Lex/ModuleMapFile.cpp clang/include/clang/Basic/Module.h clang/include/clang/Lex/ModuleMap.h clang/lib/Lex/ModuleMap.cpp clang/test/Modules/export_as_test.c

Although I believe you need to fetch those commits first.

[llvmbot]

@llvm/issue-subscribers-clang-format

Author: Michael Spencer (Bigcheese)

The formatting suggestion here is wrong: https://github.com//pull/119740#issuecomment-2539753204

It's basically suggesting:

namespace N {
class C {
  // ...
  };
  } // namespace N
  void topLevelDecl() {}

This should be reproducible with:
git-clang-format --diff 4b825c7417f72ee88ee3e4316d0c01ed463f1241 11faee533d07aff6955d44bda0d502888e52e119 --extensions cpp,c,h -- clang/include/clang/Lex/ModuleMapFile.h clang/lib/Lex/ModuleMapFile.cpp clang/include/clang/Basic/Module.h clang/include/clang/Lex/ModuleMap.h clang/lib/Lex/ModuleMap.cpp clang/test/Modules/export_as_test.c

Although I believe you need to fetch those commits first.

[owenca]

git-clang-format does this now:

$ ./clang-format -version
clang-format version 20.0.0git (https://github.com/owenca/llvm-project 38b8ebca922a7511d9f2bc048f3df0c9863cafc8)
$ git-clang-format --binary ./clang-format --diff 4b825c7417f72ee88ee3e4316d0c01ed463f1241 11faee533d07aff6955d44bda0d502888e52e119 --extensions cpp,c,h -- clang/include/clang/Lex/ModuleMapFile.h clang/lib/Lex/ModuleMapFile.cpp clang/include/clang/Basic/Module.h clang/include/clang/Lex/ModuleMap.h clang/lib/Lex/ModuleMap.cpp clang/test/Modules/export_as_test.c
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 6182d1c7f889..a689a43469c7 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -1458,73 +1458,72 @@ bool ModuleMap::resolveConflicts(Module *Mod, bool Complain) {
 //----------------------------------------------------------------------------//
 
 namespace clang {
-  class ModuleMapParser {
-    modulemap::ModuleMapFile &MMF;
-    SourceManager &SourceMgr;
-
-    DiagnosticsEngine &Diags;
-    ModuleMap ⤅
-
-    /// The current module map file.
-    FileID ModuleMapFID;
-
-    /// Source location of most recent parsed module declaration
-    SourceLocation CurrModuleDeclLoc;
-
-    /// The directory that file names in this module map file should
-    /// be resolved relative to.
-    DirectoryEntryRef Directory;
-
-    /// Whether this module map is in a system header directory.
-    bool IsSystem;
-
-    /// Whether an error occurred.
-    bool HadError = false;
-
-    /// The active module.
-    Module *ActiveModule = nullptr;
-
-    /// Whether a module uses the 'requires excluded' hack to mark its
-    /// contents as 'textual'.
-    ///
-    /// On older Darwin SDK versions, 'requires excluded' is used to mark the
-    /// contents of the Darwin.C.excluded (assert.h) and Tcl.Private modules as
-    /// non-modular headers.  For backwards compatibility, we continue to
-    /// support this idiom for just these modules, and map the headers to
-    /// 'textual' to match the original intent.
-    llvm::SmallPtrSet<Module *, 2> UsesRequiresExcludedHack;
-
-    void handleModuleDecl(const modulemap::ModuleDecl &MD);
-    void handleExternModuleDecl(const modulemap::ExternModuleDecl &EMD);
-    void handleRequiresDecl(const modulemap::RequiresDecl &RD);
-    void handleHeaderDecl(const modulemap::HeaderDecl &HD);
-    void handleUmbrellaDirDecl(const modulemap::UmbrellaDirDecl &UDD);
-    void handleExportDecl(const modulemap::ExportDecl &ED);
-    void handleExportAsDecl(const modulemap::ExportAsDecl &EAD);
-    void handleUseDecl(const modulemap::UseDecl &UD);
-    void handleLinkDecl(const modulemap::LinkDecl &LD);
-    void handleConfigMacros(const modulemap::ConfigMacrosDecl &CMD);
-    void handleConflict(const modulemap::ConflictDecl &CD);
-    void handleInferredModuleDecl(const modulemap::ModuleDecl &MD);
-
-    /// Private modules are canonicalized as Foo_Private. Clang provides extra
-    /// module map search logic to find the appropriate private module when PCH
-    /// is used with implicit module maps. Warn when private modules are written
-    /// in other ways (FooPrivate and Foo.Private), providing notes and fixits.
-    void diagnosePrivateModules(SourceLocation StartLoc);
-
-    using Attributes = ModuleMap::Attributes;
-
-  public:
-    ModuleMapParser(modulemap::ModuleMapFile &MMF,
-                    SourceManager &SourceMgr, DiagnosticsEngine &Diags,
-                    ModuleMap &Map, FileID ModuleMapFID,
-                    DirectoryEntryRef Directory, bool IsSystem)
-        : MMF(MMF), SourceMgr(SourceMgr), Diags(Diags), Map(Map),
-          ModuleMapFID(ModuleMapFID), Directory(Directory), IsSystem(IsSystem) {}
-
-    bool parseModuleMapFile();
-  };
+class ModuleMapParser {
+  modulemap::ModuleMapFile &MMF;
+  SourceManager &SourceMgr;
+
+  DiagnosticsEngine &Diags;
+  ModuleMap &Map;
+
+  /// The current module map file.
+  FileID ModuleMapFID;
+
+  /// Source location of most recent parsed module declaration
+  SourceLocation CurrModuleDeclLoc;
+
+  /// The directory that file names in this module map file should
+  /// be resolved relative to.
+  DirectoryEntryRef Directory;
+
+  /// Whether this module map is in a system header directory.
+  bool IsSystem;
+
+  /// Whether an error occurred.
+  bool HadError = false;
+
+  /// The active module.
+  Module *ActiveModule = nullptr;
+
+  /// Whether a module uses the 'requires excluded' hack to mark its
+  /// contents as 'textual'.
+  ///
+  /// On older Darwin SDK versions, 'requires excluded' is used to mark the
+  /// contents of the Darwin.C.excluded (assert.h) and Tcl.Private modules as
+  /// non-modular headers.  For backwards compatibility, we continue to
+  /// support this idiom for just these modules, and map the headers to
+  /// 'textual' to match the original intent.
+  llvm::SmallPtrSet<Module *, 2> UsesRequiresExcludedHack;
+
+  void handleModuleDecl(const modulemap::ModuleDecl &MD);
+  void handleExternModuleDecl(const modulemap::ExternModuleDecl &EMD);
+  void handleRequiresDecl(const modulemap::RequiresDecl &RD);
+  void handleHeaderDecl(const modulemap::HeaderDecl &HD);
+  void handleUmbrellaDirDecl(const modulemap::UmbrellaDirDecl &UDD);
+  void handleExportDecl(const modulemap::ExportDecl &ED);
+  void handleExportAsDecl(const modulemap::ExportAsDecl &EAD);
+  void handleUseDecl(const modulemap::UseDecl &UD);
+  void handleLinkDecl(const modulemap::LinkDecl &LD);
+  void handleConfigMacros(const modulemap::ConfigMacrosDecl &CMD);
+  void handleConflict(const modulemap::ConflictDecl &CD);
+  void handleInferredModuleDecl(const modulemap::ModuleDecl &MD);
+
+  /// Private modules are canonicalized as Foo_Private. Clang provides extra
+  /// module map search logic to find the appropriate private module when PCH
+  /// is used with implicit module maps. Warn when private modules are written
+  /// in other ways (FooPrivate and Foo.Private), providing notes and fixits.
+  void diagnosePrivateModules(SourceLocation StartLoc);
+
+  using Attributes = ModuleMap::Attributes;
+
+public:
+  ModuleMapParser(modulemap::ModuleMapFile &MMF, SourceManager &SourceMgr,
+                  DiagnosticsEngine &Diags, ModuleMap &Map, FileID ModuleMapFID,
+                  DirectoryEntryRef Directory, bool IsSystem)
+      : MMF(MMF), SourceMgr(SourceMgr), Diags(Diags), Map(Map),
+        ModuleMapFID(ModuleMapFID), Directory(Directory), IsSystem(IsSystem) {}
+
+  bool parseModuleMapFile();
+};
 
 } // namespace clang
 
@@ -1599,7 +1598,8 @@ void ModuleMapParser::handleModuleDecl(const modulemap::ModuleDecl &MD) {
     ActiveModule = nullptr;
     const Module *TopLevelModule = nullptr;
     for (unsigned I = 0, N = MD.Id.size() - 1; I != N; ++I) {
-      if (Module *Next = Map.lookupModuleQualified(MD.Id[I].first, ActiveModule)) {
+      if (Module *Next =
+              Map.lookupModuleQualified(MD.Id[I].first, ActiveModule)) {
         if (I == 0)
           TopLevelModule = Next;
         ActiveModule = Next;
$ 

[Bigcheese]

Thanks!