[PATCH] D25617: [tablegen] Merge duplicate definitions of getMinimalTypeForRange. NFC.

[Daniel Sanders via llvm-commits]

dsanders added inline comments.


================
Comment at: utils/TableGen/Types.h:19
+/// supports values of at least 32.
+const char *getMinimalTypeForRange(uint64_t Range, unsigned MaxSize = 64);
+}
----------------
qcolombet wrote:
> dsanders wrote:
> > qcolombet wrote:
> > > I would move a bit differently with the implementation.
> > > 
> > > I would say that MaxSize is just for static assert and that it must be smaller or equal to 64.
> > The MaxSize argument is only there to avoid a functional change in this particular change. Two of the getMinimalTypeForRange() implementations (RegisterInfoEmitter.cpp and AsmWriterEmitter.cpp) didn't allow uint64_t while the third did allow it. I suspect the only reason the limitation was there is that the relevant enums haven't grown large enough to warrant uint64_t yet. I was thinking that a follow-up patch could lift this limitation and remove the MaxSize argument but I didn't want to make that functional change a prerequisite of D25618. If you prefer, then I can make all three callers allow uint64_t in this change and remove MaxSize.
> I know :).
> What I am saying is that the MaxSize should be used in the assert, but not in the if.
> I.e., I want straight line code for all the cases.
I think I see what you're saying now. You're looking for something like this:
  assert((MaxSize == 64 ? Range <= 0xFFFFFFFFFFFFFFFFULL : Range <= 0xFFFFFFFFULL) && "Enum too large");
  if (Range > 0xFFFFFFFFULL)
    return "uint64_t";
  if (Range > 0xFFFF)
    return "uint32_t";
  if (Range > 0xFF)
    return "uint16_t";
  return "uint8_t";


https://reviews.llvm.org/D25617