cmake: drop '-Winline'

[Karlson2k]

'-Winline' is more like compiler debugging. It does not provide any practical information for application or library developers, as inlining can be flipped by number of uncontrollable minor reasons.
On GCC it may result in "random" warnings, on Clang it is useless.
As 'inline' keyword in correctly supported after 382717d on all build systems, there is no need to keep this warning.

[vszakats]

Is this option causing an actual problem in the curl codebase?

[Karlson2k]

It prevents the use of the inline keyword.
It slows down the builds.
It is inconsistent across build systems (autotools/CMake).

[Karlson2k]

The key point: -Winline results are varied from "useless" (on Clang) to "random noise" (on GCC). No practical benefits.

[Karlson2k]

Builds slowdowns and code pollution could be counted as problems.
Build systems inconsistency could be counted as a problem as well.

[vszakats]

How and why would these slow down the builds?

Also with the explicit inlines just added, these warnings now seem useful to know when those don't actually work, or am I misunderstanding the purpose of this option?

autotools builds have this warning since 2245ac2 (2008). The CMake one was copied from there, to keep them in sync.

[Karlson2k]

How and why would these slow down the builds?

As you pointed out in the discussion of the #13355, extra processing in configure or CMake slow down builds, especially on Windows and macOS.

This flag requires extra processing without any benefits.

Also with the explicit inlines just added, these warnings now seem useful to know when those don't actually work, or am I misunderstanding the purpose of this option?

Actually, this option is not useful:

• 'inline' is just a hint, not a hard requirement. Compiler is still free to not inline the function and this is not an error. -Winline combined with -Werror makes no inlining a hard error, which is not aligned with C standard.
• when compiling with -O0 or with -fno-inline, compiler will not even attempt to inline inline functions and will not warn about not inlined function even with -Winline.
• for always_inline/__forceinline such warning would make sense, however there is no special warning option for this. Haven't tried, but probably compiler warns about such cases without extra options.
• as described in GCC documentation: The compiler uses a variety of heuristics to determine whether or not to inline a function. For example, the compiler takes into account the size of the function being inlined and the amount of inlining that has already been done in the current function. Therefore, seemingly insignificant changes in the source program can cause the warnings produced by -Winline to appear or disappear. The same function could be inlined or not inlined depending on architecture, compiler version or even on other code. Such situations are normal and does not indicate any problem, as inline is still just a hint.
• for Clang it is just useless: has no effect in Clang.

autotools builds have this warning since 2245ac2 (2008). The CMake one was copied from there, to keep them in sync.

Thanks. I added second commit for configure.

[vszakats]

How and why would these slow down the builds?

As you pointed out in the discussion of the #13355, extra processing in configure or CMake slow down builds, especially on Windows and macOS.

There is no extra processing here, these options are just added to a list,
along with the other options. Unlike #13355.

Actually, this option is not useful:

• 'inline' is just a hint, not a hard requirement. Compiler is still free to not inline the function and this is not an error. -Winline combined with -Werror makes no inlining a hard error, which is not aligned with C standard.

If we aim for pure C89, perhaps we should not use inline at all.
It's not the warning option that breaks the standard. Also -Werror
is an off-by-default option.

• when compiling with -O0 or with -fno-inline, compiler will not even attempt to inline inline functions and will not warn about not inlined function even with -Winline.

Makes sense.

• for always_inline/__forceinline such warning would make sense, however there is no special warning option for this. Haven't tried, but probably compiler warns about such cases without extra options.
• as described in GCC documentation: [...]
• for Clang it is just useless: has no effect in Clang.

No effect for now, might be in the future.

Our few inlines are small, so don't seem to be affected by adverse side effects. Isn't the point of this warning to make sure we keep such functions small?

If we don't care about these warnings, IOW about our inline attributes honored/ignored, what is the point in using them?

I don't see the benefit of removing this option when it costs nothing to keep it there, does no harm (we've seen no report or failure related to this), but could indicate an issue with a function too large to inline, which we explicitly asked to be inline.

[Karlson2k]

How and why would these slow down the builds?

As you pointed out in the discussion of the #13355, extra processing in configure or CMake slow down builds, especially on Windows and macOS.

There is no extra processing here, these options are just added to a list, along with the other options. Unlike #13355.

Adding options to the list is processing as well. I didn't write "checking".

Actually, this option is not useful:

• 'inline' is just a hint, not a hard requirement. Compiler is still free to not inline the function and this is not an error. -Winline combined with -Werror makes no inlining a hard error, which is not aligned with C standard.

If we aim for pure C89, perhaps we should not use inline at all.

libcurl cannot be compiled with pure C89 anyway, and uses some of the C99 features. long long is required, which is not mandatory part of C89.
Unlike long long, the inline keyword is not mandatory for building curl and just improves optimisations, when available. No drawbacks.

It's not the warning option that breaks the standard. Also -Werror is an off-by-default option.
CD/CI uses -Werror and becomes broken by any warning.
As the result, absolutely legitimate situation when any inline function is not inlined considered as broken.

• when compiling with -O0 or with -fno-inline, compiler will not even attempt to inline inline functions and will not warn about not inlined function even with -Winline.

Makes sense.

• for always_inline/__forceinline such warning would make sense, however there is no special warning option for this. Haven't tried, but probably compiler warns about such cases without extra options.
• as described in GCC documentation: [...]
• for Clang it is just useless: has no effect in Clang.

No effect for now, might be in the future.

The opposite. It had effect in the past, but it was removed because it is not useful.

Our few inlines are small, so don't seem to be affected by adverse side effects. Isn't the point of this warning to make sure we keep such functions small?

It is not only about the size of the inlined functions.
If other function B, that calls inline function A already has some inlined code, then function A can be not inlined. Therefore, if any function is inlined 99 times and one time is not inlined, you need to drop inline completely to avoid warnings (which are errors in CD/CI), thus removing optimisation for such function.

If we don't care about these warnings, IOW about our inline attributes honored/ignored, what is the point in using them?

As inline is just a hint for the compiler, it is absolutely normal that the compiler will not inline some particular function in some particular conditions on some particular architecture. It does not invalidate other situations when the function successfully inlined.

With -Werror in CD/CI, it's making inline like "force inline". If a single call on any single architecture on any single compiler version is not inlined, then inline cannot be used.
Currently, it is "force inline everywhere or do not use at all".

I don't see the benefit of removing this option when it costs nothing to keep it there, does no harm (we've seen no report or failure related to this), but could indicate an issue with a function too large to inline, which we explicitly asked to be inline.

Unlike "always_inline", the inline is not a request for inlining. It is just a suggestion.
What you described is applicable to "always_inline" functions.

As I wrote earlier, the reason of not inlining can be the function that call inline function, while in other places the same inline function is successfully inlined.

[vszakats]

If a combination of the inline use and build options is causing an
actual issue, with no way to fix it with existing options (e.g. by leaving
-Werror off, or by passing CFLAGS='-Wno-error=inline'), IMO
a better option is to offer a way to disable or override the inline
keyword, like I suggested earlier.

[Karlson2k]

To summarize: -Winline may trigger warnings in an absolutely legitimate situation, when function is not inlined as clearly allowed by C. Therefore, these warnings do not indicate any actual problem.
The same function may be inlined in some places, while it may not be inlined in other situations, depending on caller function, platform, compiler version and other things. So, while inline keyword could be beneficial for some situation, it cannot be used if any single combinations of conditions causes not inline (which is not an error). This is enforced by -Werror flag in CD/CI.
In practice, it must not be a warning message, it should be an informative message.

There are two ways to deal with it:

• remove -Winline ("noisy" on GCC, no effect on Clang)
• add -Wno-error=inline to compiler flags

I'll create an alternative PR with the second option. The first option is implemented in this PR.

[vszakats]

I still don't see where the "noise" you mention is and you haven't demonstrated a real world case when these theoretical issues actually happen. Without it, it's difficult to make a case of any action.

Regarding -Wno-error=inline I meant this as a custom option in case it happens, when it happens, not something baked in as a default.

[Karlson2k]

I still don't see where the "noise" you mention is and you haven't demonstrated a real world case when these theoretical issues actually happen. Without it, it's difficult to make a case of any action.

It is written directly in GCC documentation: "The compiler uses a variety of heuristics to determine whether or not to inline a function. For example, the compiler takes into account the size of the function being inlined and the amount of inlining that has already been done in the current function. Therefore, seemingly insignificant changes in the source program can cause the warnings produced by -Winline to appear or disappear."

This sounds like "noisy".

Regarding -Wno-error=inline I meant this as a custom option in case it happens, when it happens, not something baked in as a default.

The warning message produced in absolutely legitimate situation should not be treated as an error in any conditions.
It makes no sense to have -Werror=inline, which currently is the default value for CI/CD builders.

I'd emphasise: currently the error produced for functions not marked with always_inline, i.e. for functions allowed to be not inlined.

[Karlson2k]

A couple of examples where good inline keywords has been dropped.

• https://github.com/curl/curl/pull/2592/files#diff-15bf07dce2d21ed95fb542b02e16e4c0a46253a7e2237b002ca4295ff2dfc48dR75
• add asynch getaddrinfo support #6746 (comment)

No real harm if these functions were not inlined (while marked as inline), while having inline suggestions looks quite positive.

[Karlson2k]

-Winline does not provide any useful information.
If the function is supposed to be always inlined, it must be declared as always_inline or as a macro.

What should you do with the warning about function not inlined?

• You may force inlining by always_inline. However, what's the point to declare such function as just inline initially?
• You may drop inline from the function declaration. However, it would hardly improve the code, especially if the function is not inlined in one situation, while inlined in other situations.
• As you suggested, you may mute the warning in this specific case. But what's the point to have the warning that should be just muted?

The inline keyword must be used with functions, where inlining is preferred, but not mandatory. It's useless to have any warnings (or even errors!) for not inlined functions that are not required to be inlined.

[bagder]

long long is required

Only on 32 bit systems. Which these days probably are in a minority. It actually requires "a 64 bit type", it does not necessarily have to be long long

[bagder]

I think @Karlson2k has a point. The -Winline warning is generally not very helpful and depends on compiler specific internal heuristics we can't do much to affect.

Although we have not really experienced any problems with it so far so I view it as mostly a conceptual/theoretical problem. I personally remain an inline skeptic and think we should avoid using it, or at worst use it sparingly.

[vszakats]

I think @Karlson2k has a point. The -Winline warning is generally not very helpful and depends on compiler specific internal heuristics we can't do much to affect.

Although we have not really experienced any problems with it so far so I view it as mostly a conceptual/theoretical problem. I personally remain an inline skeptic and think we should avoid using it, or at worst use it sparingly.

I'm not in favour of the direction of introducing inline (or other controversial options), then dismantling warnings on theoretical grounds around it. Many warning types can be annoying in theory, some of them are even annoying in practice sometimes, but this rarely goes to the point to actually remove them.

Deleting the TWO inline uses from the codebase is a better fix IMO. In the relevant cases those will be inlined anyway automatically by the compiler. Another fix would be to swap them with a CURL_INLINE macro with an option to disable (or override) them.

Just a few weeks ago another PR was going overboard on enabling inline in as much compilers as possible to force using it in a few isolated cases. Now we want to disable potential warnings none has seen, on the grounds that it's the job of the compiler to decide about inlining.

What is the point of these steps?

edit: timeline:
2008: -Winline present since 2245ac2
2022: inline used in a pair of functions in easy_lock.h since 23af112
2024-02: one more inline use since cbe41d1.
No report of any issues since the first use in 2022.

[Karlson2k]

I cannot tell for historic retrospective, but nowadays inline is supported almost everywhere.
I'm not sure whether a single 64-bit compiler exists without inline support. However, with the latest additions, the code can freely use inline even when the compiler is C89-only.

While keeping header in C89-only mode could be important, especially taking into account that many C99 features are not supported (or not fully supported) even by latest C++ versions (compound literals, (nested) designated initializers, variadic macros), but the code of the library and the app can take advantage of modern C features, if keeping backward compatible with C89.

By removing a useless warning, which is not informative in practice, you will get more freedom, while it does not limit anything neither make anything worse. Compiler output would be cleaner.
If the warning does not provide any useful information, what's the point to keep it? The warning is not triggered automatically if inline is used, so it is absolutely useless. I think @bagder got my point.

Any developer is free to NOT USE the inline. However, as inline is fully supported now, it could be easier (with disabled warning) to re-use code borrowed from existing sources.

Note: the inline in the curl code is supported (and used!) since 2007 (375cdf8).

[Karlson2k]

This PR is not about any use of inline in the code.

The PR is just removing the useless warning.

[vszakats]

This sounds like a preparation step (aka "freedom") to introduce more
inline use into the codebase. This doesn't seem like a good direction
to me, regardless of how many compilers support it.

If so, perhaps you'd like to say where do you plan to use it inside curl?

Any developer is free to NOT USE the inline

Speaking of the curl codebase this isn't true. It's currently impossible
to disable using inline. For that to be true, we'd need a CURL_INLINE
macro.

[Karlson2k]

This PR is only a cleanup of useless warning, which does not provide any valuable information.

I don't have any plans to use more (any) inline keywords in curl. I just started this work because I saw inconsistency in inline interpretation across build systems.

This PR does not affect the ability of curl maintainers to reject any code that is not aligned with curl coding standards/practices.

Any developer is free to NOT USE the inline

Speaking of the curl codebase this isn't true. It's currently impossible to disable using inline. For that to be true, we'd need a CURL_INLINE macro.

Frankly, I don't see how the extra macro would help with something. Unless it is used in public API, there is no difference whether inline macro is defined to empty or CURL_INLINE is used and defined to empty. Both ways disable use of inline by compiler.

However, again, this PR is only a cleanup of the useless warning.

I don't have any plans for use of inline for curl. I don't have any plans for more curl contributions.
I'm GNU libmicrohttpd maintainer. In libmicrohttpd we use libcurl as a part of test-suite, so my contributions (since 2016) are mostly about HTTP implementation and Windows-related fixes (the platform is mostly tested by me).

[vszakats]

Frankly, I don't see how the extra macro would help with something. Unless it is used in public API, there is no difference whether inline macro is defined to empty or CURL_INLINE is used and defined to empty. Both ways disable use of inline by compiler.

Redefining a language keyword also applies to all 3rd-party/system
headers, autotools and cmake tests, including their internal ones.
It's generally better to avoid doing this due to possible side-effects.

CURL_INLINE fixes these. It's also the pattern curl uses for most
(all?) other non-standard language features.

However, again, this PR is only a cleanup of the useless warning.

"useless" is your claim. What is "useless" though? How can this be
applied to a warning option? That is didn't trigger so far? That it may
give a false positive in theory? These are speculations, even
considering the disclaimer in the gcc doc.

Warning options are there because they may detect an issue. We
cannot tell that this or any particular warning will or will not do that.

Deleting it would save 1 line and 2 words in the codebase. It doesn't
affect build or runtime performance, and haven't caused false
positives. Keeping it costs nothing [1]. It may even detect a
problem when somebody accidentally tries to inline a large function.

I'd err on the side of caution and keep it, unless proven to be causing
so much issues that it outweights any possible advantage.

[1] This wasn't always so: before refactoring PickyWarnings.cmake,
each warning option was individually detected with a feature test, thus
each causing a delay on every cmake configure run.)

[Karlson2k]

Frankly, I don't see how the extra macro would help with something. Unless it is used in public API, there is no difference whether inline macro is defined to empty or CURL_INLINE is used and defined to empty. Both ways disable use of inline by compiler.

Redefining a language keyword also applies to all 3rd-party/system headers, autotools and cmake tests, including their internal ones. It's generally better to avoid doing this due to possible side-effects.

CURL_INLINE fixes these. It's also the pattern curl uses for most (all?) other non-standard language features.

On one hand, for third-party and system headers, it would make sense.
On the other hand, if the compiler does not support 'inline' at all, any use of the 'inline' directly in the headers would break build completely, right?

Either way could be implemented now, as my PR #13355 was merged.

Anyway, this is out of the scope of the PR.

However, again, this PR is only a cleanup of the useless warning.

"useless" is your claim. What is "useless" though? How can this be applied to a warning option? That is didn't trigger so far? That it may give a false positive in theory? These are speculations, even considering the disclaimer in the gcc doc.

Warning options are there because they may detect an issue. We cannot tell that this or any particular warning will or will not do that.

Actually, you can tell it if you read the warning description.
For clarity: the warning may warn you about function not inlined for function not required to be inlined.
It is like having the warning for not unrolled loop, while the loop is not required to be unrolled. Useless.

Deleting it would save 1 line and 2 words in the codebase. It doesn't affect build or runtime performance, and haven't caused false positives. Keeping it costs nothing [1]. It may even detect a problem when somebody accidentally tries to inline a large function.

Firstly, you would not allow even small inline function, not talking about large inline function.
Secondly, what will happen if a very large function unreasonably marked with 'inline' is not inlined? Anything crashed? It would cause performance problems? The build would fail?

[vszakats]

Firstly, you would not allow even small inline function, not talking about large inline function. Secondly, what will happen if a very large function unreasonably marked with 'inline' is not inlined? Anything crashed? It would cause performance problems? The build would fail?

It's useless use of inline.

[Karlson2k]

Firstly, you would not allow even small inline function, not talking about large inline function. Secondly, what will happen if a very large function unreasonably marked with 'inline' is not inlined? Anything crashed? It would cause performance problems? The build would fail?

It's useless use of inline.

Or maybe not, if it would be inlined on some particular architecture with performance benefit. Either now or with future compiler versions. You cannot tell it for sure.
At the same time, you may get warning for "useful" use of inline (if you think that inline is useless only in some specific cases, then it means inline in other cases is "useful"), just because of combination of uncontrollable factors (probably limited to single architecture or compiler version only).
So it looks more like a useless warning.

[bagder]

Thanks for your contribution and efforts @Karlson2k, but I don't think we need to merge this change. It does not fix anything that's broken.

[Karlson2k]

@bagder I got your point. However, I still think that the use of this option is a broken behaviour. I thought you have the similar position.