[llvm-dev] MSVC warning noise on "LLVM_ATTRIBUTE_ALWAYS_INLINE inline void foo()"

[Aaron Ballman via llvm-dev]

On Mon, Dec 21, 2015 at 6:40 AM, Johan Engelen <jbc.engelen at gmail.com> wrote:
> On Mon, Dec 21, 2015 at 12:08 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Sun, Dec 20, 2015 at 5:57 PM, Johan Engelen <jbc.engelen at gmail.com>
>> wrote:
>> >
>> > Perhaps LLVM_ATTRIBUTE_ALWAYS_INLINE could be defined to "inline" if the
>> > compiler has no support for always_inline (currently it is set to
>> > nothing in
>> > that case) ?
>> > I think this would allow removal of the "inline" after
>> > LLVM_ATTRIBUTE_ALWAYS_INLINE.
>>
>> Wouldn't this cause functions with MSVC that are marked
>> LLVM_ATTRIBUTE_ALWAYS_INLINE but not 'inline' to not be inlined?
>
>
> __forceinline means that MSVC will always inline that function, that is why
> the extra "inline" results in a warning.

__forceinline is still just a hint to the compiler to do the inline,
but it is a stronger hint that inline
(https://msdn.microsoft.com/en-us/library/bw1hbe6y.aspx). inline is a
keyword in C++ that has additional semantics that __forceinline is not
required to have, such as the fact that an inline function with
external linkage is required to have the same address in all TUs. I am
not comfortable assuming that inline === __forceinline for all
versions of MSVC (including future ones).

~Aaron


>
> I propose:
> in llvm/Support/Compiler.h
>
>   #if __has_attribute(always_inline) || LLVM_GNUC_PREREQ(4, 0, 0)
>   #define LLVM_ATTRIBUTE_ALWAYS_INLINE __attribute__((always_inline))
>   #elif defined(_MSC_VER)
>   #define LLVM_ATTRIBUTE_ALWAYS_INLINE __forceinline
>   #else
> - #define LLVM_ATTRIBUTE_ALWAYS_INLINE
> + #define LLVM_ATTRIBUTE_ALWAYS_INLINE inline
>   #endif
>
> and elsewhere
>
>   LLVM_ATTRIBUTE_ALWAYS_INLINE
> - inline bool operator==(StringRef LHS, StringRef RHS) {
> + bool operator==(StringRef LHS, StringRef RHS) {
>     return LHS.equals(RHS);
>   }
>
> As far as I can tell from online documentation, that will do the correct
> thing on MSVC and GCC. For non-MSVC, non-GCC, this will add "inline" in
> front of some functions that do not have it right now, notably member
> functions that are defined in the class definition (see e.g. StringRef.h
> empty()). I will have to test if /that/ doesn't create a warning ;)
>
>  -Johan
>
>