[PATCH] D63583: [Support] Add fs::getUmask() function and change fs::setPermissions
Alex Brachet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 20 11:05:05 PDT 2019
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
More information about the llvm-commits
mailing list