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