[PATCH] D73891: [RISCV] Support experimental/unratified extensions

[Lewis Revill via Phabricator via cfe-commits]

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