[PATCH] D46341: [llvm-objcopy] Add --discard-all (-x) option

[Paul Semel via Phabricator via llvm-commits]

paulsemel added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:344
+    Obj.SymbolTable->removeSymbols([&](Symbol &Sym) {
+      if (Config.DiscardAll && Sym.Binding == STB_LOCAL &&
+          Sym.getShndx() != SHN_UNDEF && Sym.Type != STT_FILE &&
----------------
alexshap wrote:
> paulsemel wrote:
> > alexshap wrote:
> > >   return Config.DiscardAll && Sym.Binding == STB_LOCAL &&
> > >              Sym.getShndx() != SHN_UNDEF && Sym.Type != STT_FILE &&
> > >              Sym.Type != STT_SECTION
> > Actually, as there is other options that will use this `removeSymbols` function, this choice was made to make it clearer. I was thinking about something like :
> > 
> > ```
> > if (Config.DiscardAll && Blabla...)
> >     return true;
> > 
> > if (Config.DiscardLocals && Blabla..)
> >     return true;
> > 
> > return false;
> > ```
> > 
> > I personally find it easier to read, even if this is not the most optimized way for doing it. What do you think ? 
> i think having 
>     if (expression which evaluates to bool) 
>         return true;
>     return false
> is kinda strange because "if" and the second "return" can be eliminated (there is no actual branching here), thus i would prefer the less verbose form, clang-format (don't forget -style=llvm) will format the long expression in a nice way, so it's not a big issue. That's my preference, but i don't really insist.
Sure, I understand that we can eliminate the condition, of course. My point was more that I find it "nicer", and, as I mentioned, there will be other conditions for other options, and I truly think that it might be ugly to have only one big `return` statement.
About the clang-format, I'm pretty sure I am doing it well, I tried again on this part of the code, but there was no changes :)


Repository:
  rL LLVM

https://reviews.llvm.org/D46341