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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.


================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:49-50
+  /// Parse RISCV ISA info from arch string.
+  static std::unique_ptr<RISCVISAInfo>
+  parseArchString(llvm::Error &Error, StringRef Arch,
+                  bool EnableExperimentalExtension,
----------------
Use llvm::Expected<...> as the return value to avoid a separate error out param


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40
+
+static const RISCVSupportedExtensionInfo SupportedExtensionInfos[] = {
+    {"i", RISCVExtensionVersion{2, 0}}, {"e", RISCVExtensionVersion{1, 9}},
----------------
Is the "Infos" suffix on the variable name really needed?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:41
+static const RISCVSupportedExtensionInfo SupportedExtensionInfos[] = {
+    {"i", RISCVExtensionVersion{2, 0}}, {"e", RISCVExtensionVersion{1, 9}},
+    {"m", RISCVExtensionVersion{2, 0}}, {"a", RISCVExtensionVersion{2, 0}},
----------------
I'd keep these all one per line


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:107
+struct FindByName {
+  FindByName(StringRef Ext) : Ext(Ext){};
+  StringRef Ext;
----------------



================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:378
+        llvm::find_if(ExtensionInfos, FindByName(ExtName));
+    if (ExtensionInfoIterator == ExtensionInfos.end())
+      continue;
----------------
No error?..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168