[PATCH] D46424: [RISCV] Support .option push and .option pop

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks Lewis, this looks good to me - just a couple of very minor requested tweaks noted inline.



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:128
+  bool popFeatureBits() {
+    if (!FeatureBitStack.size())
+      return true;
----------------
`if (FeatureBitStack.empty())` would be a ever so slightly more readable.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:131
+
+    auto FeatureBits = FeatureBitStack.pop_back_val();
+    copySTI().setFeatureBits(FeatureBits);
----------------
LLVM is quite conservative about the use of `auto`, though this isn't always adhered to throughout LLVM <https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable>. Might be worth using an explicit type here.


================
Comment at: test/MC/RISCV/option-invalid.s:18
 .option bar
+
+# CHECK: error: .option pop with no .option push
----------------
Can you add tests for .option push and .option pop with extra tokens at the end of the input? e.g. `.option pop 2`?


Repository:
  rL LLVM

https://reviews.llvm.org/D46424