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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
