lewis-revill added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:93
+ // If experimental extension, require use of current version number number
+ if (auto ExperimentalExtension = isExperimentalExtension(Ext)) {
+ if (!Args.hasArg(options::OPT_menable_experimental_extensions)) {
----------------
Hmm... Not sure it makes sense anymore for the function to be called
`isExperimentalExtension`, because it's not just returning true or false. Just
off the top of my head I would call it something like
`getExperimentalExtensionVersion` or ideally something more concise.
================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:100
+ return false;
+ } else if (Major.empty() && Minor.empty()) {
+ std::string Error =
----------------
Is just testing `Major.empty()` not sufficient? We can't have a version with
just a minor version number.
================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:125
+ // Allow extensions to declare no version number
+ if (Major.empty() && Minor.empty())
+ return true;
----------------
Ditto.
================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:366
default:
// Currently LLVM supports only "mafdc".
D.Diag(diag::err_drv_invalid_riscv_ext_arch_name)
----------------
Should this comment be updated?
================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:102
- // TODO: Handle extensions with version number.
+ // If experimental extension, require use of current version number number
+ if (auto ExperimentalExtension = isExperimentalExtension(Ext)) {
----------------
Typo 'number number' ? Also perhaps not accurate to say 'current version
number', as opposed to something like 'a supported version number'.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73891/new/
https://reviews.llvm.org/D73891
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
