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

[Yaron Dinkin via Phabricator via llvm-commits]

ydinkin 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; };
+
----------------
ruiu wrote:
> 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 `,`.
You're right, I'll rename the struct.
However, I'd like to preserve the distinction between set values and bitmasks, 
how about enableMask, disableMask & accessBits? If you prefer, we can go for enabledMask/disabledMask - but the infinitive seems better, personally.



================
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()) {
----------------
ruiu wrote:
> 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.
The pointers are the most elegant I could find since they allow to both set the correct field & preserve the information of whether we're currently enabling or disabling flags.
In order to avoid using pointers, we'd have to not only keep uint32_t for the later assign (as you suggested), but also a bool so we can track how they should be assigned.



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D69829