[split-file] Add flags to support comments and adding extension to output files

[Endill]

Summary

• --allow-comments cuts part name at the colon if one is present
• --add-file-extension adds specified extension to output files

We will change tests under clang/test/CXX/drs to use //--- dr164: 16 instead of // dr164: 16.
With --add-file-extension=.cpp --allow-comments, split-file will create a file named dr164.cpp instead of dr164: 16.
I go into more details on motivation in Discourse thread.

[aaron.ballman]

Should this also update https://llvm.org/docs/TestingGuide.html#extra-files so we have some public documentation for the functionality?

llvm/utils/split-file/split-file.cpp

47

52

138–139

Should we be kind and look at addFileExtension.getValue()[0] to see if it's . already? e.g., the user does: --add-file-extension=.cpp

[Endill]

Address Aaron's feedback

[Endill]

In D150990#4362447, @aaron.ballman wrote:

Should this also update https://llvm.org/docs/TestingGuide.html#extra-files so we have some public documentation for the functionality?

I don't think the features I implements blend in the motivating example of "Extra files" section.
I'd say we deserve our own subsection, e.g. "Grouping small tests in a single file", where we can showcase our use case, but that requires lit side of things (%{for-each-file}) to be merged as well. So I'd like to address this in a separate patch. Feel free to remind me if I get lazy or forget about this :)

llvm/utils/split-file/split-file.cpp

138–139

That's a good suggstion

[MaskRay]

I go into more details on motivation in Discourse thread.

Thanks for the link, but it seems that the thread doesn't give examples how --allow-comments and --add-file-extension are supposed to be used.

// dr164: 16 becomes //--- dr164: 16, which produces dr164.cpp file with the corresponding test.

I am confused by this comment. Does the option ignore everything starting from the :? Why?
The Discourse thread gives a link to https://github.com/llvm/llvm-project/blob/main/clang/test/CXX/drs/dr5xx.cpp which uses this notation, but the discourse thread doesn't elaborate on the syntax. This kind of detail is actually what I expect from the commit message/Phabricator summary, instead of some information embedded in a very long C++ DR specific forum post.

For testing, use llvm/test/tools/split-file/ and ninja check-llvm-tools-split-file.

[Endill]

In D150990#4365708, @MaskRay wrote:

I go into more details on motivation in Discourse thread.

Thanks for the link, but it seems that the thread doesn't give examples how --allow-comments and --add-file-extension are supposed to be used.

It does. Here's the link I used there: https://gist.github.com/Endilll/1d8035d42bdf0dc73eaaf8a8c343bf8c/revisions?diff=unified
There I diff current notation and notation we strive for.

// dr164: 16 becomes //--- dr164: 16, which produces dr164.cpp file with the corresponding test.

I am confused by this comment. Does the option ignore everything starting from the :? Why?

Yes, it ignores everything starting from the :, because in our use case that's where part name ends, and where additional data begins (which clang version implements the DR).

The Discourse thread gives a link to https://github.com/llvm/llvm-project/blob/main/clang/test/CXX/drs/dr5xx.cpp which uses this notation, but the discourse thread doesn't elaborate on the syntax. This kind of detail is actually what I expect from the commit message/Phabricator summary, instead of some information embedded in a very long C++ DR specific forum post.

What you can see today in dr5xx.cpp is a notation that we've been using for years and happy about. In that thread I go into why we need the change this patch is a part of, and why in this particular form. Explaining that would be copy-pasting large chunks from "Use case" section of that thread. I can do that for commit description if you find that helpful.

For testing, use llvm/test/tools/split-file/ and ninja check-llvm-tools-split-file.

Thank you, I'll try that.

[aaron.ballman]

In D150990#4365708, @MaskRay wrote:

// dr164: 16 becomes //--- dr164: 16, which produces dr164.cpp file with the corresponding test.

I am confused by this comment. Does the option ignore everything starting from the :? Why?

The current DR script works by looking for magic comments of the form // drNNNN: MM where NNNN is the DR # and MM is the version of Clang it's implemented in (or specifies it's not implemented, is partial, etc; there are non-numeric values for that). The idea behind this patch is to allow this to work with split-file commands so that we can have one test file with hundreds of DRs tested in it, but we can hermetically test some of those DRs to avoid interactions between tests (which happens today unfortunately). In order to support that need, we need some way to specify a file extension (otherwise the file ends up with no extension and we need to add -x c++ to the RUN lines which is unnatural and distracting) and we need some way to tell split-file that the splitting happens at a slightly differently than normal so that --- dr123: 16 doesn't try to generate a file named dr123: 16. --add-file-extension serves the first purpose and --allow-comments serves the second.

[MaskRay]

Thanks for the reply. The motivation looks good!

Those two features allow us to integrate split-file notation and C++ DR tests notation:
dr164: 16 becomes --- dr164: 16, which produces dr164.cpp file with the corresponding test.
I go into more details on motivation in Discourse thread.

I think this paragraph can be made clearer without being overly verbose:

We will change some tests under clang/test/CXX/drs to use //--- dr164: 16 instead of // dr164.
With --add-file-extension=.cpp --allow-comments, split-file will create a file named dr164.cpp instead of dr164: 16.

[Endill]

In D150990#4368703, @MaskRay wrote:

I think this paragraph can be made clearer without being overly verbose:

We will change some tests under clang/test/CXX/drs to use //--- dr164: 16 instead of // dr164.
With --add-file-extension=.cpp --allow-comments, split-file will create a file named dr164.cpp instead of dr164: 16.

Used it for description with minor edits. Thank you!

[MaskRay]

LG once the test is added :)

[Endill]

Add tests
@MaskRay please let me know if I missed anything

[MaskRay]

LGTM.

[aaron.ballman]

LGTM!

llvm/test/tools/split-file/empty-with-comments.test

+# RUN: split-file --no-leading-lines --allow-comments %s %t
+# RUN: count 0 < %t/empty
+
+#--- empty: comment

llvm/test/tools/split-file/empty-with-extension.test

+# RUN: split-file --no-leading-lines --add-file-extension=txt %s %t
+# RUN: count 0 < %t/empty.txt
+# RUN: rm -rf %t
+# RUN: split-file --no-leading-lines --add-file-extension=.txt %s %t
+# RUN: count 0 < %t/empty.txt
+
+#--- empty

llvm/test/tools/split-file/help.test

 RUN: split-file --help 2>&1 | FileCheck --implicit-check-not='General Options:' %s
 CHECK: OVERVIEW: Split input {{.*}}
 CHECK: USAGE: split-file [options] filename directory
 CHECK: Generic Options:
 CHECK: split-file Options:
+CHECK:   --add-file-extension=<ext>
+CHECK:   --allow-comments
+CHECK:   --leading-lines
 CHECK:   --no-leading-lines

llvm/utils/split-file/split-file.cpp

 static cl::opt<bool> leadingLines("leading-lines",
                                     cl::desc("Preserve line numbers"),
                                     cl::cat(cat));
 
 static cl::opt<bool> noLeadingLines("no-leading-lines",
                                     cl::desc("Don't preserve line numbers (default)"),
                                     cl::cat(cat));
 
+static cl::opt<std::string> addFileExtension("add-file-extension",
+                                             cl::desc("Add an extension to output file names"),
+                                             cl::value_desc("ext"),
+                                             cl::cat(cat));
+
+static cl::opt<bool> allowComments("allow-comments",
+                                   cl::desc("Allow comments to be introduced with a colon after the part name"),
+                                   cl::cat(cat));
+
 static StringRef toolName;
 static int errorCount;
 
 [[noreturn]] static void fatal(StringRef filename, const Twine &message) {
   if (filename.empty())
     WithColor::error(errs(), toolName) << message << '\n';
   else
     WithColor::error(errs(), toolName) << filename << ": " << message << '\n';
static int handle(MemoryBuffer &inputBuf, StringRef input) {
   for (line_iterator i(inputBuf, /*SkipBlanks=*/false, '\0'); !i.is_at_eof();) {
     const int64_t lineNo = i.line_number();
     const StringRef line = *i++;
     const size_t markerLen = line.startswith("//") ? 6 : 5;
     if (!(line.size() >= markerLen &&
           line.substr(markerLen - 4).startswith("--- ")))
       continue;
     separator = line.substr(0, markerLen);
-    const StringRef partName = line.substr(markerLen);
+    const StringRef partName = [&] {
+      StringRef partName = line.substr(markerLen);
+      if (allowComments) {
+        auto colonPos = partName.find(':');
+        if (colonPos != StringRef::npos)
+          partName = partName.substr(0, colonPos);
+      }
+      return partName;
+    }();
     if (partName.empty()) {
       error(input, lineNo, "empty part name");
       continue;
     }
     if (isSpace(partName.front()) || isSpace(partName.back())) {
       error(input, lineNo, "part name cannot have leading or trailing space");
       continue;
     }
if (errorCount)
     return 1;
   partToBegin[lastPart].end = inputBuf.getBufferEnd();
 
   std::vector<std::unique_ptr<ToolOutputFile>> outputFiles;
   SmallString<256> partPath;
   for (auto &keyValue : partToBegin) {
     partPath.clear();
     sys::path::append(partPath, output, keyValue.first);
+    if (!addFileExtension.empty()) {
+      if (addFileExtension.getValue()[0] != '.')
+        partPath.append(".");
+      partPath.append(addFileExtension.getValue());
+    }
     std::error_code ec =
         sys::fs::create_directories(sys::path::parent_path(partPath));
     if (ec)
       fatal(input, ec.message());
     auto f = std::make_unique<ToolOutputFile>(partPath.str(), ec,
                                               llvm::sys::fs::OF_None);
     if (!f)
       fatal(input, ec.message());