[PATCH] D46341: [llvm-objcopy] Add --discard-all (-x) option
Alexander Shaposhnikov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 2 09:11:35 PDT 2018
alexshap 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 &&
----------------
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.
Repository:
rL LLVM
https://reviews.llvm.org/D46341
More information about the llvm-commits
mailing list