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

[Aaron Ballman via llvm-dev]

On Mon, Dec 21, 2015 at 11:20 AM, Johan Engelen <jbc.engelen at gmail.com> wrote:
> OK, thanks for the extra explanation.
> I guess LDC will have to go the same route of silencing C4141 warnings
> (http://llvm.org/viewvc/llvm-project?view=revision&revision=247275).

That's one option. Another option might be to explore adding an
LLVM_ATTRIBUTE_INLINE_ALWAYS_INLINE macro that does the correct
combination based on compiler usage. I'm not certain whether that will
be an improvement or not (it depends on how often the pattern arises
in the code base, I would guess). Silencing the warning is a valid
option, but the unfortunate part is that the warning is for all
duplicate specifiers, not just inline.

~Aaron

>
> (sorry for the maillist noise ;)
>
> - Johan
>
>
> On Mon, Dec 21, 2015 at 3:57 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> 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
>> >
>> >
>
>