[PATCH] D64014: [Object/ELF.h] - Improve error reporting.

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

Overall I think this looks good. Others have been following the details far more closely that I've been so I'll defer to them.



================
Comment at: include/llvm/Object/ELF.h:58
+template <class ELFT>
+static std::string getSecIndex(const ELFFile<ELFT> *Obj,
+                               const typename ELFT::Shdr *Sec) {
----------------
MaskRay wrote:
> grimar wrote:
> > jhenderson wrote:
> > > I'm not sure a `static` function here makes sense?
> > I didn't want to split the definition and declaration.
> > (because it would require too many lines in a cpp for a template function:
> > 
> > ```
> > template <class ELFT>
> > std::string getSecIndex(const ELFFile<ELFT> *Obj, ...) { 
> > ...
> > }
> > ...
> > template void object::getSecIndex<ELF32LE>();
> > template void object::getSecIndex<ELF32BE>();
> > template void object::getSecIndex<ELF64LE>();
> > template void object::getSecIndex<ELF64BE>();
> > )
> > ```
> > 
> > And so there are two ways to do that correctly I think: either use
> > `inline` or `static`, otherwise the ODR would be violated.
> > 
> > It is probably can be `inline` (like other helpers in this file), I did that change.
> It makes sense. If static is used, the instances will get internal linkage and defining the template function with the same name in another translation unit will not cause a conflict (ODR violation).
Agreed. In general if it compiles and can be static you want it to be static (pending some edge cases).


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

https://reviews.llvm.org/D64014