[PATCH] D85745: emit int32 for each version part

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

If you make the second loop consistent, I'll approve, despite my concerns about the related problems.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:794
   for (int N = 0; N < 4; ++N)
-    OS.emitInt16(FrontVer.Part[N]);
+    OS.emitInt32(FrontVer.Part[N]);
 
----------------
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.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:801
               10 * LLVM_VERSION_MINOR +
               LLVM_VERSION_PATCH;
   // Clamp it for builds that use unusually large version numbers.
----------------
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.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:807
   for (int N = 0; N < 4; ++N)
     OS.emitInt16(BackVer.Part[N]);
 
----------------
Should this be `emitInt32` as well?  And again I'd use a range-based for loop.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85745/new/

https://reviews.llvm.org/D85745