[PATCH] D41397: [ELF] - Fix for ld.lld does not accept "AT" syntax for declaring LMA region
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 28 22:44:11 PST 2017
grimar added inline comments.
================
Comment at: ELF/LinkerScript.h:223-225
+ MemoryRegion *findMemoryRegion(llvm::StringRef Name);
+ MemoryRegion *pickMemoryRegion(OutputSection *Sec);
----------------
ruiu wrote:
> Well, I think you already knew what I would be saying, but these names are not good. I think you just chose "pick" because "find" is already in use. That's not a good way of naming a function.
I had to search different wording because `find` is already in use, that is obvious.
At first I tried `findNamedMemoryRegion` and `findMemoryRegion`, but did not like it. I chose `pick` because in compare with
`findMemoryRegion` which simply searches region by name, `pickMemoryRegion` uses more complex rules,
including heuristic to select the appropriate memory region for section.
After your comment I experimented with this code again, but found that I like what these methods do, so
I think problem is with naming and not with their logic/how they were splitted. Do you agree here ?
If so, what do you think about following naming ?
```
MemoryRegion *findMemoryRegion(llvm::StringRef Name);
MemoryRegion *matchMemoryRegion(OutputSection *Sec);
```
or
```
MemoryRegion *findMemoryRegion(llvm::StringRef Name);
MemoryRegion *selectMemoryRegion(OutputSection *Sec);
```
https://reviews.llvm.org/D41397
More information about the llvm-commits
mailing list