[PATCH] D118692: [llvm-objcopy][COFF] Fix section name encoding

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

> And I'm also a little unsure on how to approach testing for this, yaml2obj, however it is also missing base64 encoding which should definitely be added, but even then the yaml file ends up very large to be able to test all the different encodings. The MC counterpart of this uses llvm-mc and assembler macros to generate the very large string tables, would it be okay to do the same in llvm-objcopy testing?

If it's impractical to use yaml2obj, using llvm-mc is fine. Just make sure the appropriate `REQUIRES:` directive is included for the target.



================
Comment at: llvm/tools/llvm-objcopy/COFF/Writer.cpp:159
+    if (S.Name.size() <= COFF::NameSize) {
+      // short names can go in the field directly
       memcpy(S.Header.Name, S.Name.data(), S.Name.size());
----------------
Please make sure comments follow LLVM coding standards with upper-case first letter, and period at the end.


================
Comment at: llvm/tools/llvm-objcopy/COFF/Writer.cpp:163
+      // longer names need to go in the string table
+      char str[sizeof(S.Header.Name) + 1];
+      memset(str, 0, sizeof(S.Header.Name) + 1);
----------------
LLVM coding style uses `UpperCamelCase` for variable names. Please fix your variable names. Also, you can zero-initialise, without the need for `memset`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118692