[PATCH] D85225: [Target][AArch64] Allow for char as int8_t in AArch64AsmParser.cpp

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h:769
 
   if (std::is_same<int16_t, std::make_signed_t<T>>::value)
     return IsImm8 || IsImm16 || uint16_t(Imm & ~0xff) == Imm;
----------------
paulwalker-arm wrote:
> jyknight wrote:
> > sizeof(T) == 2 for consistency?
> sizeof(float16_t) == 2, which nicely highlights why I don't like the sizeof approach.  If not already, someday somebody we'll want to support a 1 byte type that isn't an integer.
But nobody should call these functions with T not being an integral type in the first place. It wouldn't make sense today or in the future, for what these functions do.

Actually, I'm not sure why these are even functions templated on a integer type T, rather than just passing a second "int ImmBytes" argument. At first glance, that would actually seem clearer all around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85225