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

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.

I've put a few suggestions, but generally this LGTM. Thanks!



================
Comment at: lld/ELF/ScriptParser.cpp:804
+      std::tie(withFlags, withoutFlags) = readInputSectionFlags();
+      tok = next();
+    }
----------------
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.


================
Comment at: lld/ELF/ScriptParser.cpp:712
+    InputSectionDescription *cmd =
+        readInputSectionRules(tok, withFlags, withoutFlags);
     expect(")");
----------------
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);
```



================
Comment at: lld/ELF/ScriptParser.cpp:720
+  }
+  return readInputSectionRules(tok, withFlags, withoutFlags);
 }
----------------
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);
```


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

https://reviews.llvm.org/D72756