[PATCH] D46424: [RISCV] Support .option push and .option pop
Alex Bradbury via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 12 06:40:37 PST 2018
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
More information about the llvm-commits
mailing list