[PATCH] D73107: [llvm-objcopy][COFF] Add support for --set-section-flags

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:409
 
 .. option:: --set-section-flags <section>=<flag>[,<flag>,...]
 
----------------
This is still under the "ELF-specific Options" section...


================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:415-416
 
- Following is a list of supported flags and their effects:
+ Supported flag names are `alloc`, `load`, `noload`, `readonly`, `exclude`, `debug`,
+ `code`, `data`, `rom`, `share`, `contents`, `merge` and `strings`. Not all flags are
+ meaningful for all object file formats.
----------------
These two lines need reflowing. We tend to try to keep lines within 80 characters in this document.


================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:419
+
+ Following is a list of flag effects on ELF objects:
 
----------------
I feel like we should also document the effects on COFF objects. Otherwise, people aren't going to know what each setting does.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:220
+    for (Section &Sec : Obj.getMutableSections()) {
+      const auto It = Config.SetSectionFlags.find(Sec.Name);
+      if (It != Config.SetSectionFlags.end())
----------------
sdmitriev wrote:
> jhenderson wrote:
> > Any particular reason you're making this `const`?
> Nothing specific, but I do not see any reasons why it cannot be const especially if we do not plan to modify pointee . Do you see any reasons why it cannot have const qualifier?
No specific reason, except that we don't often make local variables `const` from my experience.


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

https://reviews.llvm.org/D73107