[PATCH] D63847: [MC] Add MCInstrAnalysis::evaluateMemoryOperandAddress
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 4 05:52:04 PDT 2019
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
More information about the llvm-commits
mailing list