[PATCH] D63847: [MC] Add MCInstrAnalysis::evaluateMemoryOperandAddress

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/MC/MCInstrAnalysis.h:157
+  /// true on success, and the address in Target.
+  virtual bool
+  evaluateMemoryOperandAddress(const MCInst &Inst, uint64_t Addr, uint64_t Size,
----------------
seiya wrote:
> bcain wrote:
> > It may make more sense for this to return an llvm::Optional<uint64_t>.
> It returns bool for consistency with `evaluateBranch` but I think Optional is better too. What do you think about the consistency with other method definitions? I don't have a strong opinion.
Personally, I have no issue in this becoming Optional to improve things (or possibly Expected, depending on what you want to do on failure). A later change could change the signature of `evaluateBranch`.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:537
+
+  // RIP-relative addressing
+  if (BaseReg.getReg() == X86::RIP) {
----------------
Nit: trailing full stop.


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

https://reviews.llvm.org/D63847