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

[Haojian Wu via Phabricator via llvm-commits]

hokein added inline comments.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
+
----------------
jhenderson wrote:
> hokein wrote:
> > 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. 
> This argument doesn't make much sense to me. Why are the `all_read` and `all_write` bits implementation details that shouldn't be tested when the lack of `all_exe` is?
> 
> This test is for testing `writeToOutput`. Part of `writeToOutput`'s behaviour appears to be to create a file with the `all_read` and `all_write` bits set. Therefore, we should be testing that behaviour. As there was already one issue with the permission bits of the file this method creates, and you are directly modifiyng a test to add permissions testing, I think it's justified to request testing of the other bits.
OK, I changed the test to verify the all_read and all_write bits, please take another look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652