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

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 15 14:18:18 PST 2016


qcolombet 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);
+}
----------------
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.


https://reviews.llvm.org/D25617





More information about the llvm-commits mailing list