Skip to content
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

[dcl.inline] p2 - Make it recommended practice #6428

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Eisenwave
Copy link
Contributor

I believe this should be a recommended practice paragraph because what the paragraph demands is an optional compiler hint.

Furthermore, p1 uses \keyword{inline} specifier, whereas p2 uses inline specifier, which is inconsistent.

\defnadj{inline}{function}.

\recommended
The \keyword{inline} specifier indicates to
the implementation that inline substitution of the function body at the
point of call is to be preferred to the usual function call mechanism.
An implementation is not required to perform this inline substitution at
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two lines down is a "shall" which can't be inside "recommended practice".

I agree with the general approach, but this needs more massaging.

Copy link
Contributor Author

@Eisenwave Eisenwave Aug 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about how to rephrase the latter half of the recommendation, and quite frankly, it should just be deleted. Everything stated in the second half is redundant, especially if this is located in a recommended practice paragraph.

The wording already suggests that this inline substitution is not mandatory. On top of that, with this change, the recommendation is in a recommended practice paragraph. It is crystal clear that an implementation isn't strictly required to perform this substitution. Stating so in prose is noise.

Furthermore, it is nowhere stated that not following this recommended practice somehow disables the other rules that apply to inline functions. There is no reason why the reader may assume so, and explicitly stating it is noise.

Copy link
Member

@jwakely jwakely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what is the actual recommendation here? I don't think this fits the form of Recommended practice.

@jwakely
Copy link
Member

jwakely commented Aug 7, 2023

We don't want to say the implementations should inline all functions marked inline, because that's too strong. It's a hint that the function should be considered for inlining but what does "considered" mean in terms of the standard? How do we phrase it to account for differing inlining heuristics at different optimization levels?

@Eisenwave
Copy link
Contributor Author

Eisenwave commented Aug 7, 2023

But what is the actual recommendation here? I don't think this fits the form of Recommended practice.

We don't want to say the implementations should inline all functions marked inline, because that's too strong. It's a hint that the function should be considered for inlining but what does "considered" mean in terms of the standard? How do we phrase it to account for differing inlining heuristics at different optimization levels?

Do you mean that this paragraph here can't be Recommended practice in general here, or do you mean that it needs to be softened up even more?

I think the overall form is definitely recommended practice. However, it would be worth stating something like:

This user should be able to indicate to the implementation that this substitution should be omitted for the purpose of debugging.

This would be satisfied by having a flag that disables inlining in general, or optimization level flags. It might be beyond the scope of the standard to recommend "optimization levels" to exist, or that different inlining heuristics should be used at those levels.

@jwakely
Copy link
Member

jwakely commented Aug 7, 2023

The \keyword{inline} specifier indicates to
the implementation that inline substitution of the function body at the
point of call is to be preferred to the usual function call mechanism.

What is the recommendation here? An implementation should ... what?

Maybe it could be phrased as "An implementation should treat the inline specifier as a hint to prefer inline substitution of the function body at the point of call instead of the usual function call mechanism."

But do we want to phrase that in terms of the inline specifier, rather than in terms of inline functions? Does that mean an implicitly inline function (one declared in the class definition, or using constexpr) should not be inlined, because it doesn't use the specifier?

I don't think we should bother changing this text unless the new version is clearly better, rather than just bad in different ways.

@frederick-vs-ja
Copy link
Contributor

I guess it might be better to remove normative texts about inline substitution or turn them into notes, because inline substitution shouldn't be observable in standard semantics, although it may affect results from implementation-defined or unspecified properties.

@jwakely
Copy link
Member

jwakely commented Aug 7, 2023

Yeah. Implementations should treat the inline specifier (or implicit inline-ness) as a hint that can be completely ignored at any given call site if e.g. optimization is disabled, or the compiler's unspecified heuristics decide inlining would not be beneficial.

And implementations might inline functions that aren't inline functions anyway, so the hint isn't even required in some cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants