[PATCH] D109516: [JITLink] Factor out forEachRelocation() function from addRelocations() in ELF Aarch64 backend (NFC)

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h:113-118
+  using ProcessRelocationFuncT = Error(const typename ELFT::Rela &,
+                                       const typename ELFT::Shdr &, Section &);
+
+  Error forEachRelocation(const typename ELFT::Shdr &RelSect,
+                          std::function<ProcessRelocationFuncT> F,
+                          bool ProcessDebugSections = false);
----------------
lhames wrote:
> I think forEachRelocation should be a template method here, rather than taking a std::function:
> 
> ```
> /// Func should be callable as:
> ///   Error(const typename ELFT::Rela &,
> ///         const typename ELFT::Shdr &, Section &)
> 
> template <typename RelocHandlerFunction>
> Error forEachRelocation(const typename ELFT::Shdr &RelSect,
>                         RelocHandlerFunction &&Func, bool ProcessDebugSections) {
>   ...
> }
> ```
> 
> It might be handy to provide a method version for convenience too:
> 
> ```
> template <typename ClassT, typename RelocHandlerMethod>
> Error forEachRelocation(const typename ELFT::Shdr &RelSect,
>                         ClassT *Instance, RelocHandlerMethod *Method,
>                         bool ProcessDebugSections) {
>   return forEachRelocation(
>       RelSect,
>       [Instance, Method](const typename ELFT::Rela &Rel,
>                          const typename ELFT::Shdr &Target,
>                          Section &GS) {
>         return (Instance->*Method)(Rel, Target, GS);
>       },
>       ProcessDebugSections);
> }
> ```
> 
> With that, I think the iteration below could be reduced to:
> 
> ```
>     using Base = ELFLinkGraphBuilder<ELFT>;
>     for (const auto &RelSect : Base::Sections)
>       if (Error Err =
>             Base::forEachRelocation(RelSect, this,
>                                     &ELFJITLinker_aarch64::addSingleRelocation))
>         return Err;
> ```
Agree, that's much better!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109516/new/

https://reviews.llvm.org/D109516