[PATCH] D105168: [RISCV] Unify the arch string parsing logic to to RISCVArchStringParser.

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:122
   StringRef CodeModel = getTargetOpts().CodeModel;
+  unsigned FLEN = ISAInfo.getFLEN();
   if (CodeModel == "default")
----------------
Capitalise these as FLen and XLen everywhere


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:220
+  if (ISAInfo.isSupportedExtension(Feature,
+                                   /* CheckExperimental */ true))
+    return ISAInfo.hasExtension(Feature);
----------------
is the right formatting I think?


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:230
+  unsigned XLEN = getTriple().isArch64Bit() ? 64 : 32;
+  if (auto E = ISAInfo.parse(XLEN, Features))
+    return false;
----------------
Unused variable; I think this should either be consumeError'ed or propagated up?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:644
+                               "invalid extension prefix '%s'",
+                               Ext.str().c_str());
+    }
----------------
craig.topper wrote:
> craig.topper wrote:
> > does createStringError really require going to from StringRef to std::string to char*?
> I guess it does if you use it with %s. Maybe better to use 
> 
> ```
> "invalid extension prefix '" + Ext + "'"
> ```
> 
> since the format string is really a Twine.
That will break if the user-provided extension contains a %. Sadly `.str().c_str()` is what you need.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105168/new/

https://reviews.llvm.org/D105168