-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmake: drop '-Winline' #13596
cmake: drop '-Winline' #13596
Conversation
Is this option causing an actual problem in the curl codebase? |
It prevents the use of the |
The key point: |
'-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. As 'inline' keyword in correctly supported after 382717d on all build systems, there is no need to keep this warning.
Builds slowdowns and code pollution could be counted as problems. |
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. |
As you pointed out in the discussion of the #13355, extra processing in This flag requires extra processing without any benefits.
Actually, this option is not useful:
Thanks. I added second commit for |
There is no extra processing here, these options are just added to a list,
If we aim for pure C89, perhaps we should not use
Makes sense.
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. |
Adding options to the list is processing as well. I didn't write "checking".
libcurl cannot be compiled with pure C89 anyway, and uses some of the C99 features.
The opposite. It had effect in the past, but it was removed because it is not useful.
It is not only about the size of the inlined functions.
As With
Unlike "always_inline", the As I wrote earlier, the reason of not inlining can be the function that call |
If a combination of the |
To summarize: There are two ways to deal with it:
I'll create an alternative PR with the second option. The first option is implemented in this PR. |
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 |
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".
The warning message produced in absolutely legitimate situation should not be treated as an error in any conditions. I'd emphasise: currently the error produced for functions not marked with |
A couple of examples where good
No real harm if these functions were not inlined (while marked as |
What should you do with the warning about function not inlined?
The |
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 |
I think @Karlson2k has a point. The 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 |
I'm not in favour of the direction of introducing Deleting the TWO Just a few weeks ago another PR was going overboard on enabling What is the point of these steps? edit: timeline: |
I cannot tell for historic retrospective, but nowadays 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. Any developer is free to NOT USE the Note: the |
This PR is not about any use of The PR is just removing the useless warning. |
This sounds like a preparation step (aka "freedom") to introduce more If so, perhaps you'd like to say where do you plan to use it inside curl?
Speaking of the curl codebase this isn't true. It's currently impossible |
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) This PR does not affect the ability of curl maintainers to reject any code that is not aligned with curl coding standards/practices.
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 However, again, this PR is only a cleanup of the useless warning. I don't have any plans for use of |
Redefining a language keyword also applies to all 3rd-party/system
"useless" is your claim. What is "useless" though? How can this be Warning options are there because they may detect an issue. We Deleting it would save 1 line and 2 words in the codebase. It doesn't I'd err on the side of caution and keep it, unless proven to be causing [1] This wasn't always so: before refactoring |
On one hand, for third-party and system headers, it would make sense. Either way could be implemented now, as my PR #13355 was merged. Anyway, this is out of the scope of the PR.
Actually, you can tell it if you read the warning description.
Firstly, you would not allow even small inline function, not talking about large inline function. |
It's useless use of |
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. |
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. |
@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. |
'-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.