[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