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

[Seiya Nuta via Phabricator via llvm-commits]

seiya marked an inline comment as done.
seiya 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,
----------------
jhenderson wrote:
> 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`.
That sounds good to me. I replaced bool with Optional.


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

https://reviews.llvm.org/D63847