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

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 9 inline comments as done.
abrachet added inline comments.


================
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);
----------------
jhenderson wrote:
> It's not your fault, but this dance seems ridiculous.
Agreed, there are quite a few get/set syscalls, not sure why this needed to be the case for umask.


================
Comment at: llvm/lib/Support/Unix/Path.inc:702
+
+std::error_code setPermissions(const Twine &Path, perms Permissions, bool RespectUmask) {
   SmallString<128> PathStorage;
----------------
jhenderson wrote:
> clang-format?
Yup sorry about that I should have looked over git clang-formats changes. It did not format the .inc files. This should be proper formatting now, I just copied how it is in the header file which clang-format did.


================
Comment at: llvm/unittests/Support/Path.cpp:1541
+
+TEST_F(FileSystemTest, RespectUmask) {
+#ifndef _WIN32
----------------
jhenderson wrote:
> You're capitalization of test names is inconsistent. One starts with a leading caps, and the other doesn't.
I think this is consistent with the rest of the file. getUmask is capitalized as such because it is testing a specific function, while RespectUmask is testing behavior. is_local test versus the ReadWriteFileCanReadOrWrite test. 


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

https://reviews.llvm.org/D63583