[PATCH] D23568: [RISCV 10/10] Add common fixups and relocations

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:12
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
+#include "MCTargetDesc/RISCVMCExpr.h"
 #include "llvm/MC/MCParser/MCAsmLexer.h"
----------------
This is not in alphabetical order


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:455
+    return parseOperandWithModifier(Operands);
+    break;
   }
----------------
redundant break


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:42
+  // Determine the type of the relocation
+  switch ((unsigned)Fixup.getKind()) {
+  default:
----------------
Do you need this cast here? You save a similar switch in `RISCVAsmBackend` without one


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:170
+    llvm_unreachable("Unhandled expression!");
+  }
+
----------------
Nitpick: The double nesting and duplicating of the unreachable isn't that intuitive - we might be able to simplify the control flow by initialising `FixupKind` to a sentinel value, dropping both  `else { unreachable }` blocks, and then just placing a `if (FixupKind == sentinel) { unreachable }`


https://reviews.llvm.org/D23568