[PATCH] D83748: Adding support for PCRel32GOTLoad in ELF x86 for the jitlinker

[Jared Wyles via Phabricator via llvm-commits]

jaredwy marked an inline comment as done.
jaredwy added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:409
         });
-
+        auto SymbolIndex = Rela.getSymbol(false);
         auto Symbol = Obj.getRelocationSymbol(&Rela, &SymTab);
----------------
lhames wrote:
> MaskRay wrote:
> > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> > 
> > Please spell out the exact type if it is non-obvious...
> I'm comfortable with auto being used where the variable name clearly implies valid usage, in this case as an index.
> 
> I understand there's room for differences in opinion there, but I don't want to delay this landing: We want to get it into LLVM 11 so that people can start testing/living on it.
I share langs view, to me that the name is index the type almost doesn't matter, but understand there could be a difference of opinion there. 

There is also the consideration that this just supports x86 at the moment, Given that the we want to follow up with 32 bit support and this class is going to become very templated to match the ElfFile<T> so keeping it auto will help with reducing code churn.

Are you ok with landing it as is? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83748