[PATCH] D14140: [ELF2] SECTIONS command basic support

[Denis Protivensky via llvm-commits]

denis-protivensky added inline comments.

================
Comment at: ELF/LinkerScript.cpp:250
@@ +249,3 @@
+  InSec.InputFile = next();
+  mapBraces("(", ")", [this, &InSec]() {
+    StringRef Tok = next();
----------------
ruiu wrote:
> denis-protivensky wrote:
> > ruiu wrote:
> > > I wouldn't write this in this functional style. This can be written in a plain C style if you define skip(S) which returns true if the next token is S.
> > > 
> > >   expect("(");
> > >   while (!skip(")") {
> > >     StringRef Tok = next();
> > >     if (Tok == "EXCLUDE_FILE") {
> > >     ...
> > `mapBraces` actually does the same. I can change `peek` to `skip`, but why to produce code duplication where it can be avoided? Procedural style may be more cumbersome here than the current functional implementation.
> By duplicate, do you mean this part? I don't think this is duplicate.
> 
>   expect("(");
>   while (!skip(")") {
Do you think this code:
```
expect("(");
while (!skip(")")) {
  StringRef Tok = next();
  if (Tok == "EXCLUDE_FILE") {
    expect("(");
    while (!skip(")"))
      InSec.ExcludeFiles.push_back(next());
  } else
    InSec.Names.push_back(Tok);
}
```
is simpler and more clear than the original one:
```
mapBraces("(", ")", [this, &InSec]() {
  StringRef Tok = next();
  if (Tok == "EXCLUDE_FILE")
    mapBraces("(", ")",
              [this, &InSec]() { InSec.ExcludeFiles.push_back(next()); });
  else
    InSec.Names.push_back(Tok);
}); 
```
?


http://reviews.llvm.org/D14140