[PATCH] D63583: [Support] Add fs::getUmask() function and change fs::setPermissions

[James Henderson via Phabricator via llvm-commits]

jhenderson added reviewers: aprantl, lhames.
jhenderson added a comment.

I've added a couple of other reviewers who have made recent changes to the Path files.



================
Comment at: llvm/lib/Support/Unix/Path.inc:695-698
+  // Chose arbitary new mask and ret set the umask to the old mask.
+  // umask(2) never fails so ignore the return of the second call.
+  unsigned Mask = ::umask(0);
+  (void) ::umask(Mask);
----------------
It's not your fault, but this dance seems ridiculous.


================
Comment at: llvm/lib/Support/Unix/Path.inc:702
+
+std::error_code setPermissions(const Twine &Path, perms Permissions, bool RespectUmask) {
   SmallString<128> PathStorage;
----------------
clang-format?


================
Comment at: llvm/lib/Support/Unix/Path.inc:709
+
+
   if (::chmod(P.begin(), Permissions))
----------------
Remove double-blank line.


================
Comment at: llvm/lib/Support/Windows/Path.inc:741
+
+std::error_code setPermissions(const Twine &Path, perms Permissions, bool /*RespectUmask*/) {
   SmallVector<wchar_t, 128> PathUTF16;
----------------
abrachet wrote:
> I don't know if unnamed parameters are allowed per the style guide, it isn't used by the Windows version of this function, this was the thinking behind this.
clang-format.

I think this is fine.


================
Comment at: llvm/unittests/Support/Path.cpp:1541
+
+TEST_F(FileSystemTest, RespectUmask) {
+#ifndef _WIN32
----------------
You're capitalization of test names is inconsistent. One starts with a leading caps, and the other doesn't.


================
Comment at: llvm/unittests/Support/Path.cpp:1555
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(Perms.get(), AllRWE) << "setPermissions respects on default";
+
----------------
I think you also need a test case for explicit false for respectUmask.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63583/new/

https://reviews.llvm.org/D63583