The Version struct is defined with 4 int values. There is no reason we shouldn't use emitInt32 here. This gives more flexibility (e.g. if you use an internal sub-version that happens to be big)
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
If you make the second loop consistent, I'll approve, despite my concerns about the related problems.
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp | ||
---|---|---|
794 | On Windows (where CodeViewDebug is most important), version numbers are typically four 16-bit ints (packed into two 32-bit unsigned ints), but I guess there's no harm using 32-bit signed ints for a comment. I'd suggest using a range-based for loop so we don't need to propagate the magic number 4. | |
801 | If this output were to be processed by Windows tools expecting only 16 bits per version number component, this could overflow as soon at version 33 (or 66 is they really use unsigned 16-bit ints), and we're currently at 12. I see that this clamps to a 16-bit unsigned int. Maybe the Version struct should just be four of those. | |
807 | Should this be emitInt32 as well? And again I'd use a range-based for loop. |
After some thought, I think this is not a good change. Many part of the system/toolchain expects each part to be 16-bit.
On Windows (where CodeViewDebug is most important), version numbers are typically four 16-bit ints (packed into two 32-bit unsigned ints), but I guess there's no harm using 32-bit signed ints for a comment.
I'd suggest using a range-based for loop so we don't need to propagate the magic number 4.