[PATCH] D45284: [RISCV] More validations on the input value of -march=

[Alex Bradbury via Phabricator via cfe-commits]

asb added inline comments.


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:50
+
+static void getExtensionVersion(StringRef In, std::string &Version) {
+  auto I = In.begin();
----------------
You should probably document the limitation that this doesn't currently parse minor versions e.g. i2p0.


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:144-146
+      // Currently LLVM does not support 'e'.
+      D.Diag(diag::err_drv_invalid_riscv_arch_name)
+        << MArch << "unsupported standard user-level extension 'e'";
----------------
This could be tightened up by also rejected rv64e as invalid.


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:201-202
+      if (StdExtsItr == StdExtsEnd) {
+        size_t Pos;
+        if (hasExtension(StdExts, std::string(1, c), Pos)) {
+          D.Diag(diag::err_drv_invalid_riscv_ext_arch_name)
----------------
I'd suggest either just using StringRef::contains and getting rid of hasExtension, or adding a doc comment to hasExtension to explain its semantics.

It might also be worth adding a comment to explain why you want to check the extension is present in the StdExts string (e.g. We have reached the end of the StdExts string. Either the current extension was given outside of the canonical order (in which case issue an error), or else no canonical ordering is defined meaning no error should be generated'.


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:211
       // Move to next char to prevent repeated letter.
       ++StdExtsItr;
 
----------------
Won't this now iterate StdExtsItr past StdExtsEnd if StdExtsItr == StdExtsEnd but the hasExtension call is false?


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:265-267
     if (HasD && !HasF)
-      D.Diag(diag::err_drv_invalid_arch_name) << MArch;
+      D.Diag(diag::err_drv_invalid_riscv_arch_name) << MArch
+        << "d requires f extension to also be specified";
----------------
Add a TODO about other dependencies perhaps? e.g. ef and efd are invalid, and q requires rv64.


https://reviews.llvm.org/D45284