[clang-format] Add -r option for recursing into directories #160299
Conversation
The option recursively replaces any directories on the command-line with all entries within the specified directories, making it easier to reformat an entire directory tree. Fixes llvm#62108
jh7370
requested review from
EugeneZelenko,
HazardyKnusperkeks,
mkurdej and
owenca
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: James Henderson (jh7370) ChangesThe option recursively replaces any directories on the command-line with all entries within the specified directories, making it easier to reformat an entire directory tree. Fixes #62108 Full diff: https://github.com/llvm/llvm-project/pull/160299.diff 4 Files Affected:
diff --git a/clang/docs/ClangFormat.rst b/clang/docs/ClangFormat.rst
index 92af06e5083d6..cd00bedefa21b 100644
--- a/clang/docs/ClangFormat.rst
+++ b/clang/docs/ClangFormat.rst
@@ -93,6 +93,7 @@ to format C/C++/Java/JavaScript/JSON/Objective-C/Protobuf/C# code.
--output-replacements-xml - Output replacements as XML.
--qualifier-alignment=<string> - If set, overrides the qualifier alignment style
determined by the QualifierAlignment style flag
+ -r - Recursively format files in any specified directories
--sort-includes - If set, overrides the include sorting behavior
determined by the SortIncludes style flag
--style=<string> - Set coding style. <string> can be:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fb429a675476d..e64ef490d292a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -532,6 +532,7 @@ clang-format
literals.
- Add ``Leave`` suboption to ``IndentPPDirectives``.
- Add ``AllowBreakBeforeQtProperty`` option.
+- Add ``-r`` option for recursing into specified directories when formatting.
libclang
--------
diff --git a/clang/test/Format/recursive.test b/clang/test/Format/recursive.test
new file mode 100644
index 0000000000000..f0f3b9e5a3f07
--- /dev/null
+++ b/clang/test/Format/recursive.test
@@ -0,0 +1,21 @@
+RUN: rm -rf %t && mkdir %t
+RUN: split-file %s %t
+
+RUN: clang-format -i -r --verbose %t 2>&1 | FileCheck %s -DDIR=%t -DSEP=%{fs-sep}
+
+CHECK: Formatting [1/4] [[DIR]][[SEP]]1.cpp
+CHECK: Formatting [2/4] [[DIR]][[SEP]]2[[SEP]]3.cpp
+CHECK: Formatting [3/4] [[DIR]][[SEP]]2[[SEP]]4[[SEP]]5.cpp
+CHECK: Formatting [4/4] [[DIR]][[SEP]]2[[SEP]]6.cpp
+
+#--- 1.cpp
+int x;
+
+#--- 2/3.cpp
+int x;
+
+#--- 2/4/5.cpp
+int x;
+
+#--- 2/6.cpp
+int x;
diff --git a/clang/tools/clang-format/ClangFormat.cpp b/clang/tools/clang-format/ClangFormat.cpp
index 5f6502f7f18a8..8267417e311f0 100644
--- a/clang/tools/clang-format/ClangFormat.cpp
+++ b/clang/tools/clang-format/ClangFormat.cpp
@@ -131,6 +131,10 @@ static cl::opt<std::string> Files(
cl::desc("A file containing a list of files to process, one per line."),
cl::value_desc("filename"), cl::init(""), cl::cat(ClangFormatCategory));
+static cl::opt<bool> Recursive("r",
+ cl::desc("Recursively format files in any specified directories"),
+ cl::cat(ClangFormatCategory));
+
static cl::opt<bool>
Verbose("verbose", cl::desc("If set, shows the list of processed files"),
cl::cat(ClangFormatCategory));
@@ -700,6 +704,31 @@ int main(int argc, const char **argv) {
errs() << "Clang-formatting " << LineNo << " files\n";
}
+ if (Recursive) {
+ SmallVector<std::string> ExpandedNames;
+ for (const std::string &Path : FileNames) {
+ if (sys::fs::is_directory(Path)) {
+ std::error_code ErrorCode;
+ for (sys::fs::recursive_directory_iterator I(Path, ErrorCode), E;
+ I != E && !ErrorCode; I.increment(ErrorCode)) {
+ bool Result = false;
+ ErrorCode = sys::fs::is_regular_file(I->path(), Result);
+ // Conservatively assume that any unopenable entries are also regular
+ // files. Later code will emit an error when trying to format them, if
+ // they aren't valid by then.
+ if (ErrorCode || Result)
+ ExpandedNames.push_back(I->path());
+ }
+ } else {
+ ExpandedNames.push_back(std::move(Path));
+ }
+ }
+
+ FileNames.clear();
+ for (const std::string &Name : ExpandedNames)
+ FileNames.push_back(Name);
+ }
+
if (FileNames.empty()) {
if (isIgnored(AssumeFileName))
return 0;
|
|
@llvm/pr-subscribers-clang-format Author: James Henderson (jh7370) ChangesThe option recursively replaces any directories on the command-line with all entries within the specified directories, making it easier to reformat an entire directory tree. Fixes #62108 Full diff: https://github.com/llvm/llvm-project/pull/160299.diff 4 Files Affected:
diff --git a/clang/docs/ClangFormat.rst b/clang/docs/ClangFormat.rst
index 92af06e5083d6..cd00bedefa21b 100644
--- a/clang/docs/ClangFormat.rst
+++ b/clang/docs/ClangFormat.rst
@@ -93,6 +93,7 @@ to format C/C++/Java/JavaScript/JSON/Objective-C/Protobuf/C# code.
--output-replacements-xml - Output replacements as XML.
--qualifier-alignment=<string> - If set, overrides the qualifier alignment style
determined by the QualifierAlignment style flag
+ -r - Recursively format files in any specified directories
--sort-includes - If set, overrides the include sorting behavior
determined by the SortIncludes style flag
--style=<string> - Set coding style. <string> can be:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fb429a675476d..e64ef490d292a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -532,6 +532,7 @@ clang-format
literals.
- Add ``Leave`` suboption to ``IndentPPDirectives``.
- Add ``AllowBreakBeforeQtProperty`` option.
+- Add ``-r`` option for recursing into specified directories when formatting.
libclang
--------
diff --git a/clang/test/Format/recursive.test b/clang/test/Format/recursive.test
new file mode 100644
index 0000000000000..f0f3b9e5a3f07
--- /dev/null
+++ b/clang/test/Format/recursive.test
@@ -0,0 +1,21 @@
+RUN: rm -rf %t && mkdir %t
+RUN: split-file %s %t
+
+RUN: clang-format -i -r --verbose %t 2>&1 | FileCheck %s -DDIR=%t -DSEP=%{fs-sep}
+
+CHECK: Formatting [1/4] [[DIR]][[SEP]]1.cpp
+CHECK: Formatting [2/4] [[DIR]][[SEP]]2[[SEP]]3.cpp
+CHECK: Formatting [3/4] [[DIR]][[SEP]]2[[SEP]]4[[SEP]]5.cpp
+CHECK: Formatting [4/4] [[DIR]][[SEP]]2[[SEP]]6.cpp
+
+#--- 1.cpp
+int x;
+
+#--- 2/3.cpp
+int x;
+
+#--- 2/4/5.cpp
+int x;
+
+#--- 2/6.cpp
+int x;
diff --git a/clang/tools/clang-format/ClangFormat.cpp b/clang/tools/clang-format/ClangFormat.cpp
index 5f6502f7f18a8..8267417e311f0 100644
--- a/clang/tools/clang-format/ClangFormat.cpp
+++ b/clang/tools/clang-format/ClangFormat.cpp
@@ -131,6 +131,10 @@ static cl::opt<std::string> Files(
cl::desc("A file containing a list of files to process, one per line."),
cl::value_desc("filename"), cl::init(""), cl::cat(ClangFormatCategory));
+static cl::opt<bool> Recursive("r",
+ cl::desc("Recursively format files in any specified directories"),
+ cl::cat(ClangFormatCategory));
+
static cl::opt<bool>
Verbose("verbose", cl::desc("If set, shows the list of processed files"),
cl::cat(ClangFormatCategory));
@@ -700,6 +704,31 @@ int main(int argc, const char **argv) {
errs() << "Clang-formatting " << LineNo << " files\n";
}
+ if (Recursive) {
+ SmallVector<std::string> ExpandedNames;
+ for (const std::string &Path : FileNames) {
+ if (sys::fs::is_directory(Path)) {
+ std::error_code ErrorCode;
+ for (sys::fs::recursive_directory_iterator I(Path, ErrorCode), E;
+ I != E && !ErrorCode; I.increment(ErrorCode)) {
+ bool Result = false;
+ ErrorCode = sys::fs::is_regular_file(I->path(), Result);
+ // Conservatively assume that any unopenable entries are also regular
+ // files. Later code will emit an error when trying to format them, if
+ // they aren't valid by then.
+ if (ErrorCode || Result)
+ ExpandedNames.push_back(I->path());
+ }
+ } else {
+ ExpandedNames.push_back(std::move(Path));
+ }
+ }
+
+ FileNames.clear();
+ for (const std::string &Name : ExpandedNames)
+ FileNames.push_back(Name);
+ }
+
if (FileNames.empty()) {
if (isIgnored(AssumeFileName))
return 0;
|
|
The request for this has been open for two and a half years. I've needed it several times myself and I've not seen a convincing argument that we shouldn't support this (writing a script that recurses a directory tree to run clang-format on each level is not a one line command invocation). There is prior art in the form of things like the go formatter, plus plenty of other tools recurse through directory trees, either when explicitly asked or by default, so I think we should do it. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
See #62108 (comment). I agreed with @mkurdej then and haven't changed my mind. Let's see what @mydeveloperday thinks. |
The reason I put this up for review is because there is quite considerable demand for the feature, based on both the ticket and the linked Stack Overflow post. Writing a script or crafting a find command to walk the directory tree and gather the desired files is non-zero work, which most people don't want to do. Depending on a user's setup, it may not even be that simple. Given the clear demand and relatively stragihtforward implementation, it seems silly to tell people simply "no, we refuse to do this". That's hardly listening to what the community want, which, since clang-format is probably one of the best known tools within the wider llvm project after clang itself, we should be doing. Anyway, the clang-format-ignore file allows ignoring files from a set passed in, I assume so that things like |
|
I've seen this request many times, and I feel it would be helpful sometimes, but it's not really our job. Especially when you can use find etc If others felt it was ok I wouldn't oppose but the arguments of the original authors still remain |
|
if we were going to do this... wouldn't something like --glob '**/dir/*.cpp' be nicer to support? than doing just -r . clang-format --glob '/src/*.cpp' --glob '/include/*.h' |
I'm on Windows, so I don't have access to Also, if it's not our job, why does .clang-format-ignore exist? It feels like it's as much our job (or not) to ignore some set of files as it is to find them.
I could certainly look at that and it would fit my specific needs, though I imagine the implementation would be more complex, so I'd rather wait for buy-in before doing that work. This would avoid the issue I ran into with |
|
Its not that I don't agree with you..see https://reviews.llvm.org/D29039 (from 2017-19) , but if you look at this review, Daniel was saying he didn't feel it was our place. Its hard for us to reverse those decisions, especially made my the inventor of cf. But if you could get @owenca and @HazardyKnusperkeks to agree then I'd definitely say, those who don't want to use it, don't,those that do go for it. I would investigate a more sophisticated method like globs (as LLVM has a GlobPattern class) but it should allow multiple extensions '{.cpp,directory/**/.h}' in one go (like prettier can do)... but I'd want something better than just -r in my view |
|
I actually wanted to use that option 3 days ago, but it didn't exist. ;) |
Thanks for the comments. It may be a while, but I'll try and look for a smarter alternative (I do agree that a glob pattern would be nicer from a user's perspective). |
Take a look here
I've no idea if its any good but it might help... I think if we can have a solution that is more than just -r then I would feel more comfortable like @HazardyKnusperkeks it might just be more acceptable especially as tools like prettier support this, it kind of gives us a precident. (see https://globster.xyz/) for examples |
|
Running the new clang-format from this patch on This is not very helpful and can be dangerous. IMO clang-format should keep treating directories as invalid filenames: BTW, you can recursively list all files under a directory with |
Yes, indeed, that's why I think the option would typically best be paired with .clang-format-ignore files. Fun fact: when developing this, because we didn't have a .clang-format-ignore file locally at the time, I accidentally reformatted the entire contents of my .git directory, rather trashing my ability to do anything with the repo! This is also why I'm leaning more towards the glob pattern approach that was suggested as an alternative.
Thanks, useful to know. |
5 participants
The option recursively replaces any directories on the command-line with all entries within the specified directories, making it easier to reformat an entire directory tree.
Fixes #62108