[PATCH] D73254: [ELF] Rename relocateOne() to relocate() and pass `Relocation` to it

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.

Ok, LGTM.



================
Comment at: lld/ELF/Target.h:92
+    relocate(loc, rel, val);
+  }
 
----------------
MaskRay wrote:
> grimar wrote:
> > ```
> > Relocation rel{};
> > rel.expr = R_NONE;
> > rel.type = type;
> > ```
> > 
> > This will initialize the `expr` and `type` fields twice, isn't?
> > First to the default values and then to `R_NONE` and `type`.
> > Applying relocations is a path that is performance sensible.
> > I am not sure how compiler can/will optimize this place, but why don't you just want to use
> > `relocate(loc, {R_NONE, type, 0, 0, nullptr}, val);` to avoid double initialization?
> > 
> The compiler can easily detect that the duplicate initialization can be optimized out. `relocateNoSym` call sites are not sensitive (pltHeader,thunk,errata,... ; these are not frequently called).
> 
> `{R_NONE, type, 0, 0, nullptr}` => some members are of the same type. If we made a mistake reordering 2 members, it might not be easy to detect.
> 
> The explicit approach does not look very bad.
> The explicit approach does not look very bad.

Looks ugly to me :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73254