[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