[PATCH] D69829: [LLD] Extend /section to be compatible to MSVC link.exe

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/Config.h:88
+  // Access bits (readable, writable, executable) are applied as-is.
+  struct AttributeMask { uint32_t enable = 0, disable = 0, access = 0; };
+
----------------
I feel enabled/disabled are better names than enable/disable.

`access` is not actually a mask but a value itself, so `AttributeMask` might be little bit confusing.

How about this? Rename this struct SectionAttributes, and rename members `enabledFlags`, `disabledFlags` and `accessBits`.

nit: separate members by `;` instead of `,`.


================
Comment at: lld/COFF/DriverUtils.cpp:172
+static Configuration::AttributeMask parseSectionAttributeMask(StringRef s, Configuration::AttributeMask &mask) {
+  uint32_t *current = &mask.enable, *other = &mask.disable;
   for (char c : s.lower()) {
----------------
I'd copy values instead of taking pointers, and assign back to `mask.enable` and `mask.disable` at the end of this function, so that we can eliminate `*`s.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D69829