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

[Daniel Sanders via llvm-commits]

dsanders added inline comments.


================
Comment at: utils/TableGen/Types.cpp:17
+const char *llvm::getMinimalTypeForRange(uint64_t Range, unsigned MaxSize) {
+  if (MaxSize > 32) {
+    assert(Range <= 0xFFFFFFFFFFFFFFFFULL && "Enum too large");
----------------
qcolombet wrote:
> Get rid of that if.
 


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


https://reviews.llvm.org/D25617