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

[Daniel Sanders via llvm-commits]

dsanders added a comment.

Thanks



================
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:
> > > 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";
> Yep, that's it.
> Also add a MaxSize <= 64 assert.
I've added a MaxSize <= 64 assert in the commit.


https://reviews.llvm.org/D25617