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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1839
+
+  // FIXME: overload evaluateAsAbsolute() for uint64_t type?
+  int64_t Value;
----------------
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?


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1844
+
+  assert(Value >= 0);
+  Res = static_cast<uint64_t>(Value);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102220