[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; };
+
----------------
ydinkin wrote:
> 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.
> 
enableMask/disableMask/accessBits sounds good to me.


================
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()) {
----------------
ydinkin wrote:
> 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.
> 
OK, if you prefer pointers I'm fine with that.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D69829