Fix createUniqueFile() to do what it is documented to do: create the unique file in a temporary directory if the model isn't absolute. Fix the callers of createUniqueFile() which were affected by this change. Add test cases to verify patch.

[dirty]

Summary

Fix createUniqueFile() to do what it is documented to do: create the unique file in a temporary directory if the model isn't absolute. Fix the callers of createUniqueFile() which were affected by this change. Add test cases to verify patch.

[chfast]

I'm not sure about the change. I think both cases (creating a file in the current and in the system temp directory) should be supported.

The example in createUniqueFile() docs should be updated too.

unittests/Support/Path.cpp

509

You need to use path::append here. A path returned by system_temp_directory() never has trailing separator.

[dirty]

• Integrate feedback from chfast: use path::append() to guarantee path separator is present.

[dirty]

Well, the current interface for createUniqueFile() doesn't allow me to select current or temp directory behavior. Are you proposing that I change it?

Because if you wanted to create a file in the current directory, you could just use fs::make_absolute() and pass it relative path. That's what I did for all the clients of createUniqueFile() who wanted a file in the current directory.

What do you mean by "the example in createUniqueFile() docs should be updated too"?

unittests/Support/Path.cpp

509

Strange. On my system (OS X) system_temp_directory() has a trailing separator. But I can see why I might not be able to rely on that on all platforms. I've updated the test case to use append.

[chfast]

About the example. Currently it is:
clang-%%-%%-%%-%%-%%.s => clang-a0-b1-c2-d3-e4.s.
With your changes it should be something like:
clang-%%-%%-%%-%%-%%.s => /tmp/clang-a0-b1-c2-d3-e4.s.

In general we have 3 options here:

1. Leave the implementation as it was, fix the docs. There are definitely use cases (exposed by your patch) that would benefit from that.
2. Apply your patch. I suspect it is the case you are interested in.
3. Try to support 1 and 2. It can be by adding additional param to the function or by adding similar function.

I'm not sure which solution is the best so I would like someone else to comment on that.

unittests/Support/Path.cpp

509

I was quite sure the docs of system_temp_directory() stated that it would not have the trailing /. I will look at that later, but I cannot test OSX myself.

I would say having the trailing dir separator is at least not guaranteed.

[• rafael]

The documentation should be changed.

The common use of this is to create a temp file that is then renamed. There is no point is having to form an absolute path to do that.

[dirty]

Based on feedback from Rafael, I'll update the documentation and abandon this patch.

include/llvm/Support/FileSystem.h

 /// [0-9a-f]. If \a model is not an absolute path, a suitable temporary
 /// directory will be prepended.
 ///
 /// Example: clang-%%-%%-%%-%%-%%.s => clang-a0-b1-c2-d3-e4.s
 ///
 /// This is an atomic operation. Either the file is created and opened, or the
 /// file system is left untouched.
 ///
-/// The intendend use is for files that are to be kept, possibly after
+/// The intended use is for files that are to be kept, possibly after
 /// renaming them. For example, when running 'clang -c foo.o', the file can
 /// be first created as foo-abc123.o and then renamed.
 ///
 /// @param Model Name to base unique path off of.
 /// @param ResultFD Set to the opened file's file descriptor.
 /// @param ResultPath Set to the opened file's absolute path.
 /// @returns errc::success if Result{FD,Path} have been successfully set,
 ///          otherwise a platform-specific error_code.

lib/Object/ArchiveWriter.cpp

writeSymbolTable(raw_fd_ostream &Out, object::Archive::Kind Kind,
   return BodyStartOffset + 4;
 }
 
 std::pair<StringRef, std::error_code>
 llvm::writeArchive(StringRef ArcName,
                    std::vector<NewArchiveIterator> &NewMembers,
                    bool WriteSymtab, object::Archive::Kind Kind,
                    bool Deterministic, bool Thin) {
+  SmallString<128> AbsolutePath(ArcName);
+  if (auto EC = sys::fs::make_absolute(AbsolutePath))
+    return std::make_pair(ArcName, EC);
+
   SmallString<128> TmpArchive;
   int TmpArchiveFD;
-  if (auto EC = sys::fs::createUniqueFile(ArcName + ".temp-archive-%%%%%%%.a",
+  if (auto EC = sys::fs::createUniqueFile(AbsolutePath + ".temp-archive-%%%%%%%.a",
                                           TmpArchiveFD, TmpArchive))
     return std::make_pair(ArcName, EC);
 
   tool_output_file Output(TmpArchive, TmpArchiveFD);
   raw_fd_ostream &Out = Output.os();
   if (Thin)
     Out << "!<thin>\n";
   else

lib/Support/FileOutputBuffer.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/Path.h"
 #include <system_error>
 
 #if !defined(_MSC_VER) && !defined(__MINGW32__)
 #include <unistd.h>
 #else
 #include <io.h>
 #endif
 
if (EC)
     return EC;
 
   unsigned Mode = sys::fs::all_read | sys::fs::all_write;
   // If requested, make the output file executable.
   if (Flags & F_executable)
     Mode |= sys::fs::all_exe;
 
   // Create new file in same directory but with random name.
-  SmallString<128> TempFilePath;
+  SmallString<128> TempFilePath, AbsolutePath(FilePath);
   int FD;
-  EC = sys::fs::createUniqueFile(Twine(FilePath) + ".tmp%%%%%%%", FD,
+
+  AbsolutePath += ".tmp%%%%%%%";
+
+  EC = sys::fs::make_absolute(AbsolutePath);
+  if (EC)
+    return EC;
+  EC = sys::fs::createUniqueFile(Twine(AbsolutePath), FD,
                                  TempFilePath, Mode);
   if (EC)
     return EC;
 
   sys::RemoveFileOnSignal(TempFilePath);
 
 #ifndef LLVM_ON_WIN32
   // On Windows, CreateFileMapping (the mmap function on Windows)

lib/Support/Path.cpp

if (EC)
     return EC;
   Result = Status.getUniqueID();
   return std::error_code();
 }
 
 std::error_code createUniqueFile(const Twine &Model, int &ResultFd,
                                  SmallVectorImpl<char> &ResultPath,
                                  unsigned Mode) {
-  return createUniqueEntity(Model, ResultFd, ResultPath, false, Mode, FS_File);
+  return createUniqueEntity(Model, ResultFd, ResultPath, true, Mode, FS_File);
 }
 
 std::error_code createUniqueFile(const Twine &Model,
                                  SmallVectorImpl<char> &ResultPath) {
   int Dummy;
-  return createUniqueEntity(Model, Dummy, ResultPath, false, 0, FS_Name);
+  return createUniqueEntity(Model, Dummy, ResultPath, true, 0, FS_Name);
 }
 
 static std::error_code
 createTemporaryFile(const Twine &Model, int &ResultFD,
                     llvm::SmallVectorImpl<char> &ResultPath, FSEntity Type) {
   SmallString<128> Storage;
   StringRef P = Model.toNullTerminatedStringRef(Storage);
   assert(P.find_first_of(separators) == StringRef::npos &&

unittests/Support/Path.cpp

TEST_F(FileSystemTest, TempFiles) {
   // Make sure it exists.
   ASSERT_TRUE(sys::fs::exists(Twine(TempPath)));
 
   // Create another temp tile.
   int FD2;
   SmallString<64> TempPath2;
   ASSERT_NO_ERROR(fs::createTemporaryFile("prefix", "temp", FD2, TempPath2));
   ASSERT_TRUE(TempPath2.endswith(".temp"));
+  ASSERT_TRUE(!TempPath2.endswith("..temp"));
   ASSERT_NE(TempPath.str(), TempPath2.str());
 
   fs::file_status A, B;
   ASSERT_NO_ERROR(fs::status(Twine(TempPath), A));
   ASSERT_NO_ERROR(fs::status(Twine(TempPath2), B));
   EXPECT_FALSE(fs::equivalent(A, B));
 
   ::close(FD2);
#ifdef LLVM_ON_WIN32
   const char *Path216 =
     "abcdefghijklmnopqrstuvwxyz7abcdefghijklmnopqrstuvwxyz6"
     "abcdefghijklmnopqrstuvwxyz5abcdefghijklmnopqrstuvwxyz4"
     "abcdefghijklmnopqrstuvwxyz3abcdefghijklmnopqrstuvwxyz2"
     "abcdefghijklmnopqrstuvwxyz1abcdefghijklmnopqrstuvwxyz0";
   ASSERT_NO_ERROR(fs::createTemporaryFile(Path216, "", TempPath));
   ASSERT_NO_ERROR(fs::remove(Twine(TempPath)));
 #endif
+
+  SmallString<64> AbsolutePath;
+  sys::path::system_temp_directory(true, AbsolutePath);
+  SmallString<64> UniquePath;
+
+  // Pass an absolute model to createUniqueFile.  The resulting
+  // path should still be absolute.
+  path::append(AbsolutePath, "unique%%%%%%.txt");
+  ASSERT_NO_ERROR(fs::createUniqueFile(Twine(AbsolutePath), UniquePath));
+  EXPECT_TRUE(path::is_absolute(UniquePath));
+
+  // Pass a relative model to createUniqueFile.  The resulting
+  // path should be absolute.
+  Twine relativePath = Twine("unique%%%%%%.txt");
+  ASSERT_NO_ERROR(fs::createUniqueFile(relativePath, UniquePath));
+  EXPECT_TRUE(path::is_absolute(UniquePath));
 }
 
 TEST_F(FileSystemTest, CreateDir) {
   ASSERT_NO_ERROR(fs::create_directory(Twine(TestDirectory) + "foo"));
   ASSERT_NO_ERROR(fs::create_directory(Twine(TestDirectory) + "foo"));
   ASSERT_EQ(fs::create_directory(Twine(TestDirectory) + "foo", false),
             errc::file_exists);
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "foo"));