[PATCH] D53623: Rename InputSectionDescription SectionPattern.

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 24 02:14:05 PDT 2018


grimar added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:40
   void addPltSymbols(InputSection &IS, uint64_t Off) const override;
-  void addPltHeaderSymbols(InputSection &ISD) const override;
+  void addPltHeaderSymbols(InputSection &Pat) const override;
   bool needsThunk(RelExpr Expr, RelType Type, const InputFile *File,
----------------
This just should have been named `IS` initially it seems.


================
Comment at: lld/ELF/LinkerScript.h:146
 // Also it may be surrounded with SORT() command, so contains sorting rules.
-struct SectionPattern {
-  SectionPattern(StringMatcher &&Pat1, StringMatcher &&Pat2)
+struct Wildcard {
+  Wildcard(StringMatcher &&Pat1, StringMatcher &&Pat2)
----------------
`SectionPattern` -> `Wildcard` renaming looks good to me.

It also matches the linker script doc and sounds IMO better than `SectionPattern`:
https://sourceware.org/binutils/docs/ld/Input-Section-Wildcards.html#Input-Section-Wildcards


================
Comment at: lld/ELF/LinkerScript.h:161
+// https://sourceware.org/binutils/docs/ld/Input-Section-Basics.html
+struct SectionPattern : BaseCommand {
+  SectionPattern(StringRef FilePattern)
----------------
Honestly, I do not feel `SectionPattern` is a better name.

As far I remember we named it `InputSectionDescription` because it was named like that in the `ld` documentation:
https://sourceware.org/binutils/docs/ld/Input-Section-Basics.html
And I think we usually tried to follow the documented names in such cases, even if they were not ideal. Though in this case
I see probably see nothing wrong with the naming in the documentation.

What about renaming to `SectionDescription`? It is also short.


https://reviews.llvm.org/D53623





More information about the llvm-commits mailing list