[PATCH] D118793: [COFF] Move section name encoding into BinaryFormat

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

Some nits from me. Otherwise, LGTM.



================
Comment at: llvm/include/llvm/BinaryFormat/COFF.h:735
+/// Encode section name based on string table offset.
+/// The size of Out must be at least NameSize.
+bool encodeSectionName(char *Out, uint64_t Offset);
----------------
`NameSize` isn't referenced anywhere in this signature - I have to go a long way to find what it refers to. Perhaps replace `NameSize` with `the NameSize constant value` or something to that effect?


================
Comment at: llvm/lib/BinaryFormat/CMakeLists.txt:14
   XCOFF.cpp
+  COFF.cpp
 
----------------
Nit: filenames look to be in alphabetical order in this list, so move COFF.cpp much earlier.


================
Comment at: llvm/lib/BinaryFormat/COFF.cpp:1
+//===- llvm/BinaryFormat/COFF.cpp - The COFF format -------------*- C++ -*-===//
+//
----------------
Nit: No need for the "*- C++-*" bit of this line - this is only for .h files where the extension doesn't signify the language.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118793