Allow additional file suffixes/extensions considered as source in main include grouping

[furdyna]

Summary

By additional regex match, grouping of main include can be enabled in files that are not normally considered as a C/C++ source code.
For example, this might be useful in templated code, where template implementations are being held in *Impl.hpp files.
On the occassion, 'assume-filename' option description was reworded as it was misleading. It has nothing to do with style=file option and it does not influence sourced style filename.

[furdyna]

Hi, @krasimir @sylvestre.ledru , would you be able to help me with this patch? I am looking for anyone interested in reviewing this :)

[MyDeveloperDay]

I think fundamentally this seems like a reasonable idea, for those that don't need they don't need to use it.

clang/docs/ClangFormat.rst

32–33

I agree this seems wrong, it's making it sound like -assume-filename is a file that will replace .clang-format .. I don't think thats true as you correctly point out

clang/lib/Format/Format.cpp

480

I think we are missing a change to the operator== in Format.h (I notice IncludeIsMainRegex is missing from there too)

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

183

whats the default value of IncludeIsMainSourceRegex?, are we making a regex even when its empty?

could we not or the regex after if its defined

IsMainFile = FileName.endswith(".c") || FileName.endswith(".cc") ||
               FileName.endswith(".cpp") || FileName.endswith(".c++") ||
               FileName.endswith(".cxx") || FileName.endswith(".m") ||
               FileName.endswith(".mm");
if (!Style.IncludeIsMainSourceRegex.empty()){
    llvm::Regex MainFileRegex(Style.IncludeIsMainSourceRegex);
    IsMainFile |= MainFileRegex.match(FileName);    
}

clang/tools/clang-format/ClangFormat.cpp

80

This may be a little out of date and needs rebasing as I believe this reformat has already been done, but your new wording is correct

[furdyna]

Hi @MyDeveloperDay, I changed the code following your advice.

Thanks for cathing the operator==(), I was not sure if this omission was intentional or not - fixed both comparisons.

[MyDeveloperDay]

Thanks for the patch,

This LGTM, just a small nit for the future, can you ensure the diff is a full context diff with something like @git diff --cached -U999999 > patch.diff (I wanted to check you got the recent changes by @sylvestre.ledru in docs/ClangFormat.rst) but I couldn't scroll that far down, so you'll need to be sure you've rebased if you haven't done so already.

[furdyna]

@MyDeveloperDay Thanks for the review!

I did rebase it a few hours ago. I actually made sure to use the full-context diff (with -U999999 option) - I even have this as a last command in my command history, and the patch file on my disk is full-context. So either I still missed something, or the tool is playing with us ;)

BTW, I don't have rights to commit to master / merge the pull request. Can you do this for me?

clang/docs/ClangFormat.rst

.. code-block:: console
   together with <file>s, the files are edited in-place. Otherwise, the
   result is written to the standard output.
 
   USAGE: clang-format [options] [<file> ...]
 
   OPTIONS:
 
   Clang-format options:
 
     --Werror                   - If set, changes formatting warnings to errors
-    --assume-filename=<string> - When reading from stdin, clang-format assumes this
-                                 filename to look for a style config file (with
-                                 -style=file) and to determine the language.
+    --assume-filename=<string> - Override filename used to determine the language.
+                                 When reading from stdin, clang-format assumes this
+                                 filename to determine the language.
     --cursor=<uint>            - The position of the cursor when invoking
                                  clang-format from an editor integration
     --dry-run                  - If set, do not actually make the formatting changes
     --dump-config              - Dump configuration options to stdout and exit.
                                  Can be used with -style option.
     --fallback-style=<string>  - The name of the predefined style used as a
                                  fallback in case clang-format is invoked with
                                  -style=file, but can not find the .clang-format

clang/docs/ClangFormatStyleOptions.rst

**IncludeIsMainRegex** (``std::string``)
   category 0, see above), use this regex of allowed suffixes to the header
   stem. A partial match is done, so that:
   - "" means "arbitrary suffix"
   - "$" means "no suffix"
 
   For example, if configured to "(_test)?$", then a header a.h would be seen
   as the "main" include in both a.cc and a_test.cc.
 
+**IncludeIsMainSourceRegex** (``std::string``)
+  Specify a regular expression for files being formatted
+  that are allowed to be considered "main" in the
+  file-to-main-include mapping.
+
+  By default, clang-format considers files as "main" only when they end
+  with: ``.c``, ``.cc``, ``.cpp``, ``.c++``, ``.cxx``, ``.m`` or ``.mm``
+  extensions.
+  For these files a guessing of "main" include takes place
+  (to assign category 0, see above). This config option allows for
+  additional suffixes and extensions for files to be considered as "main".
+
+  For example, if this option is configured to ``(Impl\.hpp)$``,
+  then a file ``ClassImpl.hpp`` is considered "main" (in addition to
+  ``Class.c``, ``Class.cc``, ``Class.cpp`` and so on) and "main
+  include file" logic will be executed (with *IncludeIsMainRegex* setting
+  also being respected in later phase). Without this option set,
+  ``ClassImpl.hpp`` would not have the main include file put on top
+  before any other include.
+
 **IndentCaseLabels** (``bool``)
   Indent case labels one level from the switch statement.
 
   When ``false``, use the same indentation level as for the switch
   statement. Switch statement body is always indented one level more than
   case labels.
 
   .. code-block:: c++

clang/docs/ReleaseNotes.rst

- The ``Standard`` style option specifies which version of C++ should be used
 
   The previous values of ``Cpp03`` and ``Cpp11`` are deprecated. Note that
   ``Cpp11`` is treated as ``Latest``, as this was always clang-format's behavior.
   (One motivation for this change is the new name describes the behavior better).
 
 - clang-format gets a new option called ``--dry-run`` or ``-n`` to emit a
   warning.
 
+- Option *IncludeIsMainSourceRegex* added to allow for additional
+  suffixes and file extensions to be considered as a source file
+  for execution of logic that looks for "main *include* file" to put
+  it on top.
+
+  By default, clang-format considers *source* files as "main" only when
+  they end with: ``.c``, ``.cc``, ``.cpp``, ``.c++``, ``.cxx``,
+  ``.m`` or ``.mm`` extensions. This config option allows to
+  extend this set of source files considered as "main".
+            
+  For example, if this option is configured to ``(Impl\.hpp)$``,
+  then a file ``ClassImpl.hpp`` is considered "main" (in addition to
+  ``Class.c``, ``Class.cc``, ``Class.cpp`` and so on) and "main
+  include file" logic will be executed (with *IncludeIsMainRegex* setting
+  also being respected in later phase). Without this option set,
+  ``ClassImpl.hpp`` would not have the main include file put on top
+  before any other include.
+
 libclang
 --------
 
 - ...
 
 
 Static Analyzer
 ---------------

clang/include/clang/Format/Format.h

return AccessModifierOffset == R.AccessModifierOffset &&
            DerivePointerAlignment == R.DerivePointerAlignment &&
            DisableFormat == R.DisableFormat &&
            ExperimentalAutoDetectBinPacking ==
                R.ExperimentalAutoDetectBinPacking &&
            FixNamespaceComments == R.FixNamespaceComments &&
            ForEachMacros == R.ForEachMacros &&
            IncludeStyle.IncludeBlocks == R.IncludeStyle.IncludeBlocks &&
            IncludeStyle.IncludeCategories == R.IncludeStyle.IncludeCategories &&
+           IncludeStyle.IncludeIsMainRegex ==
+               R.IncludeStyle.IncludeIsMainRegex &&
+           IncludeStyle.IncludeIsMainSourceRegex ==
+               R.IncludeStyle.IncludeIsMainSourceRegex &&
            IndentCaseLabels == R.IndentCaseLabels &&
            IndentGotoLabels == R.IndentGotoLabels &&
            IndentPPDirectives == R.IndentPPDirectives &&
            IndentWidth == R.IndentWidth && Language == R.Language &&
            IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
            JavaImportGroups == R.JavaImportGroups &&
            JavaScriptQuotes == R.JavaScriptQuotes &&
            JavaScriptWrapImports == R.JavaScriptWrapImports &&

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

struct IncludeStyle {
   /// category 0, see above), use this regex of allowed suffixes to the header
   /// stem. A partial match is done, so that:
   /// - "" means "arbitrary suffix"
   /// - "$" means "no suffix"
   ///
   /// For example, if configured to "(_test)?$", then a header a.h would be seen
   /// as the "main" include in both a.cc and a_test.cc.
   std::string IncludeIsMainRegex;
+
+  /// Specify a regular expression for files being formatted
+  /// that are allowed to be considered "main" in the
+  /// file-to-main-include mapping.
+  ///
+  /// By default, clang-format considers files as "main" only when they end
+  /// with: ``.c``, ``.cc``, ``.cpp``, ``.c++``, ``.cxx``, ``.m`` or ``.mm``
+  /// extensions.
+  /// For these files a guessing of "main" include takes place
+  /// (to assign category 0, see above). This config option allows for
+  /// additional suffixes and extensions for files to be considered as "main".
+  ///
+  /// For example, if this option is configured to ``(Impl\.hpp)$``,
+  /// then a file ``ClassImpl.hpp`` is considered "main" (in addition to
+  /// ``Class.c``, ``Class.cc``, ``Class.cpp`` and so on) and "main
+  /// include file" logic will be executed (with *IncludeIsMainRegex* setting
+  /// also being respected in later phase). Without this option set,
+  /// ``ClassImpl.hpp`` would not have the main include file put on top
+  /// before any other include.
+  std::string IncludeIsMainSourceRegex;
 };
 
 } // namespace tooling
 } // namespace clang
 
 LLVM_YAML_IS_SEQUENCE_VECTOR(clang::tooling::IncludeStyle::IncludeCategory)
 
 namespace llvm {

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("DisableFormat", Style.DisableFormat);
     IO.mapOptional("ExperimentalAutoDetectBinPacking",
                    Style.ExperimentalAutoDetectBinPacking);
     IO.mapOptional("FixNamespaceComments", Style.FixNamespaceComments);
     IO.mapOptional("ForEachMacros", Style.ForEachMacros);
     IO.mapOptional("IncludeBlocks", Style.IncludeStyle.IncludeBlocks);
     IO.mapOptional("IncludeCategories", Style.IncludeStyle.IncludeCategories);
     IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
+    IO.mapOptional("IncludeIsMainSourceRegex",
+                   Style.IncludeStyle.IncludeIsMainSourceRegex);
     IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
     IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
     IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives);
     IO.mapOptional("IndentWidth", Style.IndentWidth);
     IO.mapOptional("IndentWrappedFunctionNames",
                    Style.IndentWrappedFunctionNames);
     IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
     IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

 } // anonymous namespace
 
 IncludeCategoryManager::IncludeCategoryManager(const IncludeStyle &Style,
                                                StringRef FileName)
     : Style(Style), FileName(FileName) {
   FileStem = llvm::sys::path::stem(FileName);
   for (const auto &Category : Style.IncludeCategories)
     CategoryRegexs.emplace_back(Category.Regex, llvm::Regex::IgnoreCase);
   IsMainFile = FileName.endswith(".c") || FileName.endswith(".cc") ||
                FileName.endswith(".cpp") || FileName.endswith(".c++") ||
                FileName.endswith(".cxx") || FileName.endswith(".m") ||
                FileName.endswith(".mm");
+  if (!Style.IncludeIsMainSourceRegex.empty()) {
+    llvm::Regex MainFileRegex(Style.IncludeIsMainSourceRegex);
+    IsMainFile |= MainFileRegex.match(FileName);
+  }
 }
 
 int IncludeCategoryManager::getIncludePriority(StringRef IncludeName,
                                                bool CheckMainHeader) const {
   int Ret = INT_MAX;
   for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i)
     if (CategoryRegexs[i].match(IncludeName)) {
       Ret = Style.IncludeCategories[i].Priority;
if (Err) {
       auto ErrMsg = "Unexpected conflicts in #include deletions: " +
                     llvm::toString(std::move(Err));
       llvm_unreachable(ErrMsg.c_str());
     }
   }
   return Result;
 }
 
-
 } // namespace tooling
 } // namespace clang

clang/tools/clang-format/ClangFormat.cpp

FallbackStyle("fallback-style",
                            "-style=file, but can not find the .clang-format\n"
                            "file to use.\n"
                            "Use -fallback-style=none to skip formatting."),
                   cl::init(clang::format::DefaultFallbackStyle),
                   cl::cat(ClangFormatCategory));
 
 static cl::opt<std::string> AssumeFileName(
     "assume-filename",
-    cl::desc("When reading from stdin, clang-format assumes this\n"
-             "filename to look for a style config file (with\n"
-             "-style=file) and to determine the language."),
+    cl::desc("Override filename used to determine the language.\n"
+             "When reading from stdin, clang-format assumes this\n"
+             "filename to determine the language."),
     cl::init("<stdin>"), cl::cat(ClangFormatCategory));
 
 static cl::opt<bool> Inplace("i",
                              cl::desc("Inplace edit <file>s, if specified."),
                              cl::cat(ClangFormatCategory));
 
 static cl::opt<bool> OutputXML("output-replacements-xml",
                                cl::desc("Output replacements as XML."),

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ParsesConfiguration) {
   CHECK_PARSE("IncludeCategories:\n"
               "  - Regex: abc/.*\n"
               "    Priority: 2\n"
               "  - Regex: .*\n"
               "    Priority: 1",
               IncludeStyle.IncludeCategories, ExpectedCategories);
   CHECK_PARSE("IncludeIsMainRegex: 'abc$'", IncludeStyle.IncludeIsMainRegex,
               "abc$");
+  CHECK_PARSE("IncludeIsMainSourceRegex: 'abc$'",
+              IncludeStyle.IncludeIsMainSourceRegex, "abc$");
 
   Style.RawStringFormats.clear();
   std::vector<FormatStyle::RawStringFormat> ExpectedRawStringFormats = {
       {
           FormatStyle::LK_TextProto,
           {"pb", "proto"},
           {"PARSE_TEXT_PROTO"},
           /*CanonicalDelimiter=*/"",

clang/unittests/Format/SortIncludesTest.cpp

EXPECT_EQ("#include \"a.h\"\n"
             "#include \"llvm/a.h\"\n",
             sort("#include \"b.h\"\n"
                  "#include \"a.h\"\n"
                  "#include \"c.h\"\n"
                  "#include \"llvm/a.h\"\n",
                  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, LeavesMainHeaderFirstInAdditionalExtensions) {
+  Style.IncludeIsMainRegex = "([-_](test|unittest))?|(Impl)?$";
+  EXPECT_EQ("#include \"b.h\"\n"
+            "#include \"c.h\"\n"
+            "#include \"llvm/a.h\"\n",
+            sort("#include \"llvm/a.h\"\n"
+                 "#include \"c.h\"\n"
+                 "#include \"b.h\"\n",
+                 "a_test.xxx"));
+  EXPECT_EQ("#include \"b.h\"\n"
+            "#include \"c.h\"\n"
+            "#include \"llvm/a.h\"\n",
+            sort("#include \"llvm/a.h\"\n"
+                 "#include \"c.h\"\n"
+                 "#include \"b.h\"\n",
+                 "aImpl.hpp"));
+
+  // .cpp extension is considered "main" by default
+  EXPECT_EQ("#include \"llvm/a.h\"\n"
+            "#include \"b.h\"\n"
+            "#include \"c.h\"\n",
+            sort("#include \"llvm/a.h\"\n"
+                 "#include \"c.h\"\n"
+                 "#include \"b.h\"\n",
+                 "aImpl.cpp"));
+  EXPECT_EQ("#include \"llvm/a.h\"\n"
+            "#include \"b.h\"\n"
+            "#include \"c.h\"\n",
+            sort("#include \"llvm/a.h\"\n"
+                 "#include \"c.h\"\n"
+                 "#include \"b.h\"\n",
+                 "a_test.cpp"));
+
+  // Allow additional filenames / extensions
+  Style.IncludeIsMainSourceRegex = "(Impl\\.hpp)|(\\.xxx)$";
+  EXPECT_EQ("#include \"llvm/a.h\"\n"
+            "#include \"b.h\"\n"
+            "#include \"c.h\"\n",
+            sort("#include \"llvm/a.h\"\n"
+                 "#include \"c.h\"\n"
+                 "#include \"b.h\"\n",
+                 "a_test.xxx"));
+  EXPECT_EQ("#include \"llvm/a.h\"\n"
+            "#include \"b.h\"\n"
+            "#include \"c.h\"\n",
+            sort("#include \"llvm/a.h\"\n"
+                 "#include \"c.h\"\n"
+                 "#include \"b.h\"\n",
+                 "aImpl.hpp"));
+}
+
 TEST_F(SortIncludesTest, RecognizeMainHeaderInAllGroups) {
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
   Style.IncludeBlocks = tooling::IncludeStyle::IBS_Merge;
 
   EXPECT_EQ("#include \"c.h\"\n"
             "#include \"a.h\"\n"
             "#include \"b.h\"\n",
             sort("#include \"b.h\"\n"