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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:127
+
+bool RISCVISAInfo::isSupportedExtensionFeature(StringRef Ext) {
+  bool IsExperimental = stripExperimentalPrefix(Ext);
----------------
This looks like a `find_if` if that'd make it any simpler.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:147
+                                        unsigned MinorVersion) {
+  for (auto const &SupportedExtensionInfo :
+       filterSupportedExtensionInfosByName(Ext)) {
----------------
This also looks like a `find_if`


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:231
+  size_t RHSLen = RHS.length();
+  int LHSRank, RHSRank;
+  if (LHSLen == 1 && RHSLen != 1)
----------------
Not sure why these need to be declared up here.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:310
+        errc::invalid_argument,
+        "Failed to parsing major version number for extension '" + Ext + "'");
+
----------------
`Failed to parse ...`


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:315
+        errc::invalid_argument,
+        "Failed to parsing minor version number for extension '" + Ext + "'");
+
----------------
`Failed to parse`


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:359
+    }
+    // return true;
+    return Error::success();
----------------
Commented-out code


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:414
+  // RISC-V ISA strings must be lowercase.
+  if (llvm::any_of(Arch, [](char C) { return isupper(C); })) {
+    return createStringError(errc::invalid_argument,
----------------
I think you can just have `if (llvm::any_of(Arch, isupper))` here.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2036
+    for (auto Feature : RISCVFeatureKV) {
+      if (llvm::RISCVISAInfo::isSupportedExtensionFeature(Feature.Key)) {
+        clearFeatureBits(Feature.Value, Feature.Key);
----------------
Don't need `{}` here


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2055
+
+    for (auto Feature : RISCVFeatureKV) {
+      if (ISAInfo.hasExtension(Feature.Key)) {
----------------
Don't need `{}` here either


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168