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

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 17 13:07:35 PST 2016


qcolombet accepted this revision.
qcolombet added a reviewer: qcolombet.
qcolombet added a comment.
This revision is now accepted and ready to land.

LGTM

One nitpick.

Thanks Daniel!



================
Comment at: utils/TableGen/Types.h:19
+/// supports values of at least 32.
+const char *getMinimalTypeForRange(uint64_t Range, unsigned MaxSize = 64);
+}
----------------
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.


https://reviews.llvm.org/D25617





More information about the llvm-commits mailing list