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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/LinkerScript.cpp:250
@@ +249,3 @@
+  InSec.InputFile = next();
+  mapBraces("(", ")", [this, &InSec]() {
+    StringRef Tok = next();
----------------
denis-protivensky wrote:
> 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);
> }); 
> ```
> ?
Yes, I do think so (as a C/C++ programmer and as a Scheme programmer). The former is much more straightforward C++ and easier to understand than the latter, especially for people who are not very familiar with all the code in this file.


http://reviews.llvm.org/D14140