We often need to iterate over linker script commands.
And very often we need only a single command type in a loop.
I suggest adding the helper for convenience.
The patch shows the change I am suggesting.
Details
- Reviewers
ruiu • espindola javed.absar
Diff Detail
Event Timeline
ELF/LinkerScript.h | ||
---|---|---|
319 | Since this only seems to be used in foreach loops maybe using llvm::make_filter_range instead of creating a new vector would make sense? |
I'm personally not too excited about this kind of higher-order function, and my first impression is that the original code is better. I might be convinced either way, but I think I'm mildly against doing this.
- Reimplemented with use of make_filter_range.
(honestly, this looking a bit scary for me, though I still like the caller side simplifications).
The original code is sometimes hard to read IMO.
For example constructions like below are simplified greatly.
for (BaseCommand *Base : SectionCommands) { auto *Sec = dyn_cast<OutputSection>(Base); if (!Sec) continue;
You do not need to think about what is Base2 anymore:
for (BaseCommand *Base2 : Sec->SectionCommands) if (auto *Cmd = dyn_cast<SymbolAssignment>(Base2)) declareSymbol(Cmd);
ELF/LinkerScript.h | ||
---|---|---|
319 | s/static/inline, no? Using auto as a return type is c++14 according to clang. This could probably use a map + filter: template <class CmdType> using Foo = llvm::filter_iterator<ArrayRef<BaseCommand *>::iterator, decltype(&dynCastToCmdType<CmdType>)>; template <class CmdType> using Bar = llvm::mapped_iterator<Foo<CmdType>, decltype(dynCastToCmdType<CmdType>) *>; template <class CmdType> llvm::iterator_range<Bar<CmdType>> filter(ArrayRef<BaseCommand *> V) { But it feels like some of this code should be in LLVM. In particular, LLVM should probably have
|
The code I have is:
template <class CmdType> CmdType *castToCmdType(BaseCommand *Cmd) { return cast<CmdType>(Cmd); } template <class CmdType> bool isaCmdType(const BaseCommand *Cmd) { return isa<CmdType>(Cmd); } template <class CmdType> using Foo = llvm::filter_iterator<ArrayRef<BaseCommand *>::iterator, decltype(&isaCmdType<CmdType>)>; template <class CmdType> using Bar = llvm::mapped_iterator<Foo<CmdType>, decltype(castToCmdType<CmdType>) *>; template <class CmdType> llvm::iterator_range<Bar<CmdType>> filter(ArrayRef<BaseCommand *> V) { auto R = llvm::make_filter_range(V, isaCmdType<CmdType>); // FIXME: llvm should have a make_mapped_range auto B = map_iterator(R.begin(), castToCmdType<CmdType>); auto E = map_iterator(R.end(), castToCmdType<CmdType>); return llvm::make_range(B, E); }
ELF/LinkerScript.h | ||
---|---|---|
319 |
I thought we can use c++14 features as long as minimal versions of clang/gcc version accept it. |
LGTM
ELF/LinkerScript.h | ||
---|---|---|
319 | Good question. It might be a good idea to ask on llvm-dev. At least the clang build is still using -std=c++11. |
- Used filter<> in a few more places.
Rui, are you OK to land this version? It reduces nesting and simplifies loops..
ELF/LinkerScript.h | ||
---|---|---|
320 | This wrapper function could also be replaced with template<typename CmdType> using CastCmdType = CmdType *(*)(BaseCommand*); And then using static_cast<CastCmdType<CmdType>>(&llvm::cast<CmdType, BaseCommand>) instead of castToCmdType<CmdType>. However, I'm not sure that's any easier to understand. |
Since this only seems to be used in foreach loops maybe using llvm::make_filter_range instead of creating a new vector would make sense?