[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D153652#4451292 <https://reviews.llvm.org/D153652#4451292>, @jhenderson wrote:

> The updated unit test is failing on Windows in the pre-merge checks. Please investigate and fix as appropriate.

Good catch, thanks. I have restricted the unittest to linux only, I think it should be fine -- the behavior on windows is a bit different, the exe bit is set even you only set `read|write` bits, the `unittests/Support/Path.cpp` verifies that behavior.



================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:499
+  ErrorOr<llvm::sys::fs::perms> Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
----------------
avl wrote:
> !!  looks a bit unclear. Probably check it in more explicit way?
> 
> EXPECT_TRUE(Perms && !(*Perms & llvm::sys::fs::all_exe)); 
sure, done.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
+
----------------
jhenderson wrote:
> Here and below, rather than just checking the all_exe bit, let's check the permissions are exactly what are expected (e.g. does it have the read/write perms?). 
checking all existing bits is a bit tricky here (I tried it, then gave up):

- createTemporaryFile() creates a file with `owner_read | owner_write`
- writeToOutput() sets the written file to `all_read | all_write`

Both API don't provide a way to customize these bits, and they're internal details. We could test against them, but testing the implementation details seems subtle. And here we aim to verify the exe-bit not set by the `writeToOutput`, so I think just testing the exe-bit is not set should be enough. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652