[PATCH] D91314: [RISCV] ELF attribute for Zfh extension.

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:335
+      case 'z':
+        if (Arch.consume_front("zfh")) {
+          Features.AddFeature("experimental-zfh");
----------------
luismarques wrote:
> Do we need to actually consume the prefix, or would using just `startswith` be fine?
For now, `startswith` would work due to the `drop_until` below. However, `consume_front` matches the `take_front` above, and will be important when we want to start parsing version numbers.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2144
+        setFeatureBits(RISCV::FeatureExtZfh, "experimental-zfh");
+        drop_char_num = 3;
       } else
----------------
jrtc27 wrote:
> Having each X and Z extension hard-code its length is going to get ugly (and this doesn't match the code style for variable names). Instead I think we should be making good use of StringRef::consume_front.
... ok but now we drop that many characters twice: once from `consume_front` and once from `drop_front` below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91314