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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 3 03:23:42 PDT 2019


jhenderson added inline comments.


================
Comment at: include/llvm/Object/ELF.h:58
+template <class ELFT>
+static std::string getSecIndex(const ELFFile<ELFT> *Obj,
+                               const typename ELFT::Shdr *Sec) {
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > jakehehrlich wrote:
> > > > 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).
> > > I found this article about "static inline vs inline vs static in C++": https://gist.github.com/htfy96/50308afc11678d2e3766a36aa60d5f75
> > > And it turns out my statement was not fully correct. I assumed that inline implies static, but it is actually not.
> > > 
> > > Going to change `inline std::string getSecIndex` to `static inline std::string getSecIndex`
> > I've been refreshing my memory of `inline`, `static` and template functions since making this statement, and turns out I was wrong (slightly). Typing it out to make sure I have got my understanding correct as much as anything else:
> > 1) `static` means you'll get a copy of this function in every translation unit, local to that translation unit (or it might be inlined).
> > 2) `inline` means there'll be a copy in every TU. If there's one or more other instances of the function, they have to be identical, and one will end up being used. If they aren't identical, it is undefined behaviour.
> > 3) Template function definitions work the same way as `inline` functions: one copy is instantiated in each TU that uses it. If there's one or more other instances of the function, they have to be identical, and if so, one will be used. If they are not identical it is undefined behaviour.
> > 
> > From this I conclude that there's no point in using `inline` in this context. There is a point in using `static` if we think somewhere else in the code-base there's a TU that doesn't reference this header file, where somebody creates an identical copy of the function signature, but with different contents. If we don't care about that situation, then I'd argue that `static` is bad because it will increase executable size (assuming it isn't inlined) more than a template function definition otherwise would. It's not obvious to me that this function would be inlined (I expect it would, but it might not be), and I don't believe any other instances of this function are likely to be defined elsewhere in the `object` namespace, so personally I don't believe either `static` or `inline` should be used in this case. The same may not be true in other cases though, and I don't care that much about it.
> > 
> > Don't forget, `inline` may or may not have an impact on inlining of the function. The compiler will make the decision regardless of its presence or not.
> Ok. After reading more about using `inline` with template functions, I agree with the part of your summary saying that "there's no point in using inline in this context". I am not sure about omitting the `static`, as makes possible to violate ODR and ELF.h is a global header, though I agree that having one more function with the same name is indeed very unlikely. Having that, I am OK to remove both `inline` and `static`.
> 
> 
Subtle ODR issues only come into play when people write a non-identical version of this function somewhere else (in the same namespace etc) not using ELF.h (if they are using ELF.h, they'll get a compiler error about duplicate definitions or something, I expect, and I think that's fine). I don't think it matters that much either way, and I don't care strongly whether we should have `static` here or not, so I'm happy to defer to others opinions.


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

https://reviews.llvm.org/D64014





More information about the llvm-commits mailing list