[PATCH] D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked 3 inline comments as done.
peter.smith added a comment.

Thanks for the comments. It has been a bit of a nightmare of a day and I've only just got to this. I'll apply the suggestions and commit tomorrow morning so I can catch any problems.



================
Comment at: lld/ELF/ScriptParser.cpp:804
+      std::tie(withFlags, withoutFlags) = readInputSectionFlags();
+      tok = next();
+    }
----------------
grimar wrote:
> grimar wrote:
> > grimar wrote:
> > > The same.
> > And here.
> This comment was missed. Here you can inline the `next()` call too:
> 
> ```
>     if (consume("INPUT_SECTION_FLAGS"))
>       std::tie(withFlags, withoutFlags) = readInputSectionFlags();
>     cmd->sectionCommands.push_back(
>         readInputSectionRules(next(), withFlags, withoutFlags));
> ```
> 
> This is what original code did, btw.
Apologies for missing it, I'll update to inline.


================
Comment at: lld/ELF/ScriptParser.cpp:712
+    InputSectionDescription *cmd =
+        readInputSectionRules(tok, withFlags, withoutFlags);
     expect(")");
----------------
grimar wrote:
> peter.smith wrote:
> > grimar wrote:
> > > I think you can now get rid of `tok = next();`:
> > > 
> > > ```
> > > InputSectionDescription *cmd =
> > >   readInputSectionRules(next(), withFlags, withoutFlags);
> > > ```
> > I don't think I can, that line is the same as line 699 in the diff on the left hand side.
> > 
> > ```
> >     StringRef filePattern = next();
> >     if (consume("INPUT_SECTION_FLAGS"))
> > ```
> > I think I can rename tok to filePattern though.
> Not sure I understand why there can be any problems with the following code?
> 
> ```
>    if (consume("INPUT_SECTION_FLAGS"))
>       std::tie(withFlags, withoutFlags) = readInputSectionFlags();
>     InputSectionDescription *cmd =
>         readInputSectionRules(next(), withFlags, withoutFlags);
> ```
> (I tried and it works)
> You do not use `tok` below the `readInputSectionRules` anyways.
> 
> The original code had the `StringRef filePattern` variable.
> I am not sure how much useful to have it,
> but at least it named the token somehow.
> 
> But in the following version
> ```
>     tok = next();
>     InputSectionDescription *cmd =
>         readInputSectionRules(tok, withFlags, withoutFlags);
> ```
> I do not see why it might be useful to do `tok = next();` and not just inline the `next()`?
> 
> So I'd suggest to write in one of the following ways:
> 
> ```
>    if (consume("INPUT_SECTION_FLAGS"))
>       std::tie(withFlags, withoutFlags) = readInputSectionFlags();
>     InputSectionDescription *cmd =
>         readInputSectionRules(next(), withFlags, withoutFlags);
> ```
> 
> or stick to the original code:
> 
> ```
>     if (consume("INPUT_SECTION_FLAGS"))
>       std::tie(withFlags, withoutFlags) = readInputSectionFlags();
>     StringRef filePattern = next();
>     InputSectionDescription *cmd =
>         readInputSectionRules(filePattern, withFlags, withoutFlags);
> ```
> 
I see now, I missed the inlining of next(), I'll update to do
```
if (consume("INPUT_SECTION_FLAGS"))
   std::tie(withFlags, withoutFlags) = readInputSectionFlags();
 InputSectionDescription *cmd =
     readInputSectionRules(next(), withFlags, withoutFlags);
```



================
Comment at: lld/ELF/ScriptParser.cpp:720
+  }
+  return readInputSectionRules(tok, withFlags, withoutFlags);
 }
----------------
MaskRay wrote:
> grimar wrote:
> > I'd suggest to early return in the "INPUT_SECTION_FLAGS" branch just like we do in the "KEEP" branch.
> > It seems probably a bit simpler to exit early here too?
> > 
> > ```
> >   if (tok == "INPUT_SECTION_FLAGS") {
> >     std::tie(withFlags, withoutFlags) = readInputSectionFlags();
> >     return readInputSectionRules(next(), withFlags, withoutFlags);
> >   }
> >   return readInputSectionRules(tok, withFlags, withoutFlags);
> > ```
> `tok = next();` looks fine to me.. The return statement will become complex with the change.
I'll keep as it is for the moment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72756/new/

https://reviews.llvm.org/D72756