[PATCH] D102220: [NFC, CFI] Use unsigned type for CFI register values in parser/streamer code

[Venkata Ramanaiah Nalamothu via Phabricator via llvm-commits]

RamNalamothu marked an inline comment as done.
RamNalamothu added inline comments.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1839
+
+  // FIXME: overload evaluateAsAbsolute() for uint64_t type?
+  int64_t Value;
----------------
scott.linder wrote:
> Looking at this again, it seems like at the same time you would make this change, you would probably just make `parseAbsoluteExpression(uint64_t)` virtual in the base class to match the signed variant? It seems odd to have an overload of `parseAbsoluteExpression` only in this derived class, so maybe a `TODO` at the declaration to clarify for future readers would be helpful?
I thought about it but while trying to limit the changes, I didn't explore it fully thinking that might trigger more changes but apparently not many changes. Will make it virtual in base class.

Yes, a TODO should have been helpful.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1844
+
+  assert(Value >= 0);
+  Res = static_cast<uint64_t>(Value);
----------------
scott.linder wrote:
> I don't think I considered the context fully here, as this will fire on pretty easy-to-write and reasonable user inputs, right? Any absolute expression in a CFA directive which ends up being negative will cause an assert.
> 
> Maybe this should be `if (Value >= 0) return Error(...);` instead?
If call sites invoke `parseAbsoluteExpression(uint64_t)` and expect a positive return value from an expression which actually yields negative value, I feel this check better be an assert so that those scenarios can be fixed appropriately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102220