jhenderson 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); + ---------------- 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. 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
