[PATCH] D146054: [RISCV] Add --print-supported-extensions support
Fangrui Song via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 17 13:28:51 PDT 2023
MaskRay added a comment.
Herald added a subscriber: sunshaoce.
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`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146054/new/
https://reviews.llvm.org/D146054
More information about the cfe-commits
mailing list