4vtomat added a comment. In D146054#4596571 <https://reviews.llvm.org/D146054#4596571>, @MaskRay wrote:
> In D146054#4587210 <https://reviews.llvm.org/D146054#4587210>, @4vtomat wrote: > >> In D146054#4586067 <https://reviews.llvm.org/D146054#4586067>, @MaskRay >> wrote: >> >>> I think the best place to test `RISCVISAInfo.cpp` is >>> `llvm/unittests/Support/RISCVISAInfoTest.cpp`. >>> >>> `clang/test/Driver/print-supported-extensions.c` can test just a few lines >>> (there will be some overlap with the testing in >>> `llvm/unittests/Support/RISCVISAInfoTest.cpp`), so that changes to RISC-V >>> extensions will generally not require updates to >>> `clang/test/Driver/print-supported-extensions.c` >> >> The goal of this patch is to list the supported extensions and their >> versions by providing an option, so I guess >> `clang/test/Driver/print-supported-extensions.c` aims differently with >> `llvm/unittests/Support/RISCVISAInfoTest.cpp` which is testing the >> functionalities in `RISCVISAInfoTest.cpp`. >> `clang/test/Driver/print-supported-extensions.c` only tracks the extensions >> added and the their version changes and `riscvExtensionsHelp` in >> `llvm/lib/Support/RISCVISAInfo.cpp` doesn't have any input or output as well >> as any side effect, it only reads `SupportedExtensions` and >> `SupportedExperimentalExtensions` and dump the information. >> So I think `clang/test/Driver/print-supported-extensions.c` is enough for >> this patch? > > My comment is about: the test is placed at the wrong layer. See > https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer > > I don't want that RISC-V development in LLVM causes repeated changes to > `clang/test/Driver`. Got it, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146054/new/ https://reviews.llvm.org/D146054 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
