[PATCH] D23566: [RISCV 8/10] Add support for all RV32I instructions

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:146
+    char Prev = '\0';
+    for (char c : Str) {
+      if (c != 'i' && c != 'o' && c != 'r' && c != 'w')
----------------
theraven wrote:
> mgrang wrote:
> > Shouldn't this be:
> > ```
> > for (char &c : Str)
> > ```
> > Refer: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> Do you really think that making a copy of a `char` and reusing it in a register is more expensive than taking a reference to a char in the middle of the string and relying on alias analysis to ensure that we only load via that pointer once?
> 
> References to small (register or pair of register) POD values are likely to have more overhead in a range-based `for` loop than copies and should only be used if you need to modify the value in place.
Thanks for the feedback Mandeep. I think using a reference is unnecessary here for the same reason you typically wouldn't use a reference when declaring a function taking an int or char argument. Making c `const` would perhaps be a better incremental improvement, but given c's short scope it wouldn't add much to readability. [I don't think LLVM has a consistent policy on declaring local PODs as const, but could be wrong]


https://reviews.llvm.org/D23566