[PATCH] D41397: [ELF] - Fix for ld.lld does not accept "AT" syntax for declaring LMA region

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/LinkerScript.h:223-225
+  MemoryRegion *findMemoryRegion(llvm::StringRef Name);
+  MemoryRegion *pickMemoryRegion(OutputSection *Sec);
 
----------------
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.


================
Comment at: ELF/ScriptParser.cpp:711
 
+  if (consume("AT")) {
+    if (!peek().startswith(">")) {
----------------
grimar wrote:
> ruiu wrote:
> > grimar wrote:
> > > ruiu wrote:
> > > > This seems unnecessarily complicated. Why don't you just not allow a space between "AT" and ">"?
> > > I checked that gnu linkers support all 4 forms:
> > > ```
> > > AT>REGION
> > > AT> REGION
> > > AT >REGION
> > > AT > REGION
> > > ```
> > > 
> > > PR mentions 2 ways: "AT> syntax" and "AT > syntax".
> > > Spec mentions third: "AT>lma_region" (https://www.eecs.umich.edu/courses/eecs373/readings/Linker.pdf, p.48).
> > > 
> > > I guess we just want to support all of them then.
> > It's a little bit pathetic. If you want to do that, please try changing the lexer so that ">" is always parsed as a single-character token instead of a regular token character
> Changing lexer feels a bit overkill for this feature for me, though looks it probably the most correct way.
> I'll try it, thanks. 
This part of code looks much nicer now.


https://reviews.llvm.org/D41397