[PATCH] D69829: [LLD] Extend /section to be compatible to MSVC link.exe
Yaron Dinkin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 5 07:23:48 PST 2019
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
More information about the llvm-commits
mailing list