[PATCH] D67750: Allow additional file suffixes/extensions considered as source in main include grouping
furdyna created this revision.
furdyna added reviewers: rsmith, ioeric.
Herald added a project: clang.
furdyna edited the summary of this revision.
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.
Repository:
rC Clang
https://reviews.llvm.org/D67750
Files:
clang/docs/ClangFormat.rst
clang/docs/ClangFormatStyleOptions.rst
clang/docs/ReleaseNotes.rst
clang/include/clang/Tooling/Inclusions/IncludeStyle.h
clang/lib/Format/Format.cpp
clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
clang/tools/clang-format/ClangFormat.cpp
clang/unittests/Format/FormatTest.cpp
clang/unittests/Format/SortIncludesTest.cpp
Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -395,6 +395,57 @@
"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;
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -12194,6 +12194,8 @@
IncludeStyle.IncludeCategories, ExpectedCategories);
CHECK_PARSE("IncludeIsMainRegex: 'abc$'", IncludeStyle.IncludeIsMainRegex,
"abc$");
+ CHECK_PARSE("IncludeIsMainSourceRegex: 'abc$'",
+ IncludeStyle.IncludeIsMainSourceRegex, "abc$");
Style.RawStringFormats.clear();
std::vector ExpectedRawStringFormats = {
Index: clang/tools/clang-format/ClangFormat.cpp
===
--- clang/tools/clang-format/ClangFormat.cpp
+++ clang/tools/clang-format/ClangFormat.cpp
@@ -72,12 +72,12 @@
cl::init(clang::format::DefaultFallbackStyle),
cl::cat(ClangFormatCategory));
-static cl::opt
-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::init(""), cl::cat(ClangFormatCategory));
+static cl::opt AssumeFileName(
+"assume-filename",
+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(""), cl::cat(ClangFormatCategory));
static cl::opt Inplace("i",
cl::des
[PATCH] D67750: Allow additional file suffixes/extensions considered as source in main include grouping
furdyna added a comment. Hi, @krasimir @sylvestre.ledru , would you be able to help me with this patch? I am looking for anyone interested in reviewing this :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67750/new/ https://reviews.llvm.org/D67750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69764: [clang-format] Add East/West Const fixer capability
furdyna added a comment. > I think its worth mentioning, that my personal preference would STILL be to > land this inside clang-format with default configuration of "OFF", > I would be interested to know how many people would be unhappy if we stated > that "sorting includes" and "namespace comments" had to be removed from > clang-format and into the new tool! > So you know, I'm pushing because I'm being ask privately to land this, > because I assume people want to use it, but maybe don't want to voice their > opinions publicly. I was a quiet observer up to this point, but I hope that my voice will help asses "what community wants", at least as an additional sample point. I also hope that the "community" is not only defined here as "open-source projects", but also all those poor souls working for closed-source projects ;) I started observing this review when I was still working for another company and we were really interested in this feature. We were using clang-format extensively on several multi-milion LOC projects with total number of developers going into hundreds. "sorting includes" and "namespace comments" were one of the features that we enabled in our code bases as soon as they landed. We even quietly helped to improve include sorting here, in clang-format, squashing some bugs or extending the use-cases. We had those features enabled even with known limitations because clang-format in such large projects is not only a "simple whitespace formatting tool" - it becomes emotionless policing tool and time-saver during code reviews. It had never crossed our minds that "sorting includes" or "namespace comments" should be separate tools. And if we did create separate tools as opposed to extending clang-format, I am 90% sure they would stay closed behind corporate doors. Another point of view is how easy it is to integrate clang-format as an existing well-known tool into various IDEs vs integrating a set of separate code-formatting programs that can have their own amusing side-effects when launched in different order. > land this inside clang-format with default configuration of "OFF", yes please. IMHO merging this and letting it mature over time as people use it is the best way forward vs risking various forks to be maintained by teams internally and spooking potential contributors > I would be interested to know how many people would be unhappy at least a few hundreds > because I assume people want to use it a cure for that itch, yes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69764/new/ https://reviews.llvm.org/D69764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67750: Allow additional file suffixes/extensions considered as source in main include grouping
furdyna updated this revision to Diff 228872.
furdyna marked 2 inline comments as done.
furdyna added a comment.
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67750/new/
https://reviews.llvm.org/D67750
Files:
clang/docs/ClangFormat.rst
clang/docs/ClangFormatStyleOptions.rst
clang/docs/ReleaseNotes.rst
clang/include/clang/Format/Format.h
clang/include/clang/Tooling/Inclusions/IncludeStyle.h
clang/lib/Format/Format.cpp
clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
clang/tools/clang-format/ClangFormat.cpp
clang/unittests/Format/FormatTest.cpp
clang/unittests/Format/SortIncludesTest.cpp
Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -466,6 +466,57 @@
"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;
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -12847,6 +12847,8 @@
IncludeStyle.IncludeCategories, ExpectedCategories);
CHECK_PARSE("IncludeIsMainRegex: 'abc$'", IncludeStyle.IncludeIsMainRegex,
"abc$");
+ CHECK_PARSE("IncludeIsMainSourceRegex: 'abc$'",
+ IncludeStyle.IncludeIsMainSourceRegex, "abc$");
Style.RawStringFormats.clear();
std::vector ExpectedRawStringFormats = {
Index: clang/tools/clang-format/ClangFormat.cpp
===
--- clang/tools/clang-format/ClangFormat.cpp
+++ clang/tools/clang-format/ClangFormat.cpp
@@ -75,9 +75,9 @@
static cl::opt 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(""), cl::cat(ClangFormatCategory));
static cl::opt Inplace("i",
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -184,6 +184,10 @@
FileName.endswith(".cpp") || FileName.endswith(".c++") ||
FileName.endswith(".cxx") || FileName.endswith(".m") ||
FileName.endswith(".mm");
+ if (!Style.IncludeIsMainSourceRegex.empty()) {
+llvm::Regex MainFileRege
[PATCH] D67750: Allow additional file suffixes/extensions considered as source in main include grouping
furdyna added a comment. @MyDeveloperDay Thanks for the review! I did rebase it a few hours ago. I actually made sure to use the full-context diff (with -U99 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? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67750/new/ https://reviews.llvm.org/D67750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
