scott.linder added inline comments.
================ Comment at: llvm/lib/MC/MCAsmStreamer.cpp:993 + // Metadata needs to be padded out to an even word size. + uint32_t PaddedSize = alignTo(std::max((int)MetadataSize, 1), 4); + uint32_t PaddingSize = PaddedSize - MetadataSize; ---------------- With the `MetadataSize == 0` case handled the `std::max` should be redundant now, right? Also the literal `4` can be `WordSize` ================ Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1018 + for (; Index + WordSize <= MetadataSize; Index += WordSize) + PrintWord((const uint8_t *)Metadata.data() + Index); + ---------------- Nit: for pointer casts I would prefer an explicit `reinterpret_cast` ================ Comment at: llvm/lib/MC/MCAsmStreamer.cpp:981 + + // Metadata needs to be padded out to an even word size. + size_t MetadataSize = Metadata.size(); ---------------- stephenpeckham wrote: > There's no requirement to pad the .info section. When you generate assembly > language, the .info pseudo-op can only generate words of data, so the if the > data length is not 0(mod 4), the last word will have to be padded with > low-order 0s. This means that the length of the .info section will be a > multiple of 4. The length, on the other hand, should be exact. At link time, > only "length" bytes will be copied to the output file. > > If you emit object code directly, you will not need to emit any padding bytes. Does the comment still need updating then? It could capture the fact that the "lowest common denominator" is the assembly syntax as it works in terms of words, and so may requiring padding in the final word. We can be clear that we apply the same restriction to the object case judiciously, as it make the output identical and avoids more code paths. We could also note that the linker can use the length to optimize the final linked binary. These all clear things up to a fresh reader, so I think they are worthwhile things to include in comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153600/new/ https://reviews.llvm.org/D153600 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
