hokein added a comment. Thanks for the comments.
In D153652#4447976 <https://reviews.llvm.org/D153652#4447976>, @jhenderson wrote: > Is there anything that can be done to use gtest unit tests for this? The two > lit tests are useful, but the problem with them is that if they switch to > using a different approach to emitting their data, the lit tests won't cover > the Support code you're actually changing. The usages of `llvm::writeOutput` are in the ToolMain.cpp files, I don't see a way to unittest them. > Some commit message nits: > >> [llvm][Support] Don'tt set "all_exe" mode by default for file written by >> llvm::writeToOutput. > > There's no need to include the `[llvm]` tag - the `[Support]` tag already > indicates the library clearly enough IMHO. Also, typo "Don'tt" -> "Don't" and > we don't usually end commit titles with a ".". Thanks, updated. > Does the clang include cleaner tool have testing that covers its usage of > this API? No, the clang-include-cleaner binary tool just literally writes the text code to an output file, it unlikes the `llvm-dwardfutil` and `llvm-objcopy` tools which has a preserving-input-file-permissions requirement. ================ Comment at: llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test:1 +# RUN: yaml2obj %p/Inputs/common.yaml -o %t.o +# RUN: chmod a+x %t.o ---------------- jhenderson wrote: > It might make sense to reuse the test code from llvm-objcopy's > mirror-permissions tests. See my other inline comment too. by reusing the code, I think you mean calling the `llvm-dwardfutil` tool in the `llvm-objcopy` lit test, I'm not sure it is a good idea, calling a different tool in the `llvm/test/tools/llvm-objcopy/mirror-permissions-*.test` seems like violating the layer. ================ Comment at: llvm/test/tools/llvm-objcopy/ELF/file-permissions.test:1 +# RUN: cp %p/Inputs/dwarf.dwo %t-exe.dwo +# RUN: chmod a+x %t-exe.dwo ---------------- jhenderson wrote: > llvm-objcopy already has testing for permissions - see > mirror-permissions-*.test and respect-umask.test. It seems like a new test > file would be a mistake. Furthermore, a casual inspection to me makes it seem > like this behaviour is already covered (and working correctly), which makes > me think that no new testing is needed for this tool, but please review the > existing test and see what you think. thanks! I didn't notice there is an existing one. Taking a look on these test, I think it already covers the cases we aim to test. So no need to add a new one. 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
