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.

Sign up for GitHub

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

Jump to bottom

[lib] Drop 'inline' from 'inline constexpr' variable templates. #4625

Closed
wants to merge 1 commit into from
Closed

[lib] Drop 'inline' from 'inline constexpr' variable templates. #4625

wants to merge 1 commit into from

Conversation

jensmaurer
Copy link
Member

Since CWG2387, constexpr variable templates have external linkage.

Fixes #4601

@zygoloid, since const(expr) variable templates now have external linkage, it seems the "inline" is now redundant. What do you think?

timsong-cpp
timsong-cpp reviewed Jun 4, 2021
View reviewed changes
source/atomics.tex Outdated Show resolved Hide resolved
@jensmaurer jensmaurer marked this pull request as draft June 4, 2021 13:47
cpplearner
cpplearner reviewed Jun 4, 2021
View reviewed changes
source/iterators.tex Outdated Show resolved Hide resolved
cpplearner
cpplearner reviewed Jun 4, 2021
View reviewed changes
source/ranges.tex Outdated Show resolved Hide resolved
@jensmaurer jensmaurer marked this pull request as ready for review June 5, 2021 17:07
@jensmaurer
Copy link
Member Author

@timsong-cpp , should be much better now.

@wg21bot wg21bot added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jun 17, 2021
@jensmaurer jensmaurer removed the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jun 17, 2021
@zygoloid
Copy link
Member

zygoloid commented Jun 17, 2021 •
edited

@zygoloid, since const(expr) variable templates now have external linkage, it seems the "inline" is now redundant. What do you think?

I agree that removing inline from constexpr variable templates is a correct change. However, if I remember correctly, the LWG folks were informed that the inline is redundant for variable templates (and it was -- arguably -- redundant before CWG2387 too, albeit for a different reason), but they wanted to keep it anyway. Seems worth checking with them.

In passing, I did a pass through the wording to double-check that inline has no other semantic effect, and I found a bug that might be construed as causing problems with this change; here it is with suggested fix:

[module.import]/6: "A header unit shall not contain a definition of a non-inline non-template function or variable whose name has external linkage."

I'm not sure whether we'd consider that editorial, but the intent of the rule is that it only applies to declarations that allow at most one definition.

@tkoeppe
Copy link
Contributor

tkoeppe commented Jun 17, 2021

However, if I remember correctly, the LWG folks were informed that the inline is redundant for variable templates (and it was -- arguably -- redundant before CWG2387 too, albeit for a different reason), but they wanted to keep it anyway. Seems worth checking with them.

Thank you, that's good to know! @jwakely, @CaseyCarter: woudl you happen to have any immediate thoughts on whether the proposed change would be desirable by LWG, or perhaps on the contrary?

@tkoeppe tkoeppe added decision-required A decision of the editorial group (or the Project Editor) is required. lwg Issue must be reviewed by LWG. labels Jun 17, 2021
@CaseyCarter
Copy link
Contributor

IIRC we were uncomfortable with "arguably redundant before CWG2387" and decided to keep the possibly-redundant inlines until such time as they became unarguably extraneous. I believe that is now the case.

While it wasn't an explicit discussion, I did recently mention this PR on the reflector (https://lists.isocpp.org/lib/2021/06/19716.php) as an argument to avoid adding another redundant inline in an LWG issue resolution, and there was no hue and cry.

@jwakely
Copy link
Member

jwakely commented Jun 18, 2021

until such time as they became unarguably extraneous

Yes, that was my understanding. "It's probably redundant, but there's an active core issue" wasn't very reassuring 🙂

@jensmaurer
Copy link
Member Author

Editorial meeting 2021-06-18: This is pending the [module.import] fix mentioned by @zygoloid.

@jensmaurer jensmaurer added changes requested Changes to the wording or approach have been requested and not yet applied. and removed decision-required A decision of the editorial group (or the Project Editor) is required. lwg Issue must be reviewed by LWG. labels Jun 18, 2021
@wg21bot wg21bot added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jun 20, 2021
@jensmaurer jensmaurer removed the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jul 4, 2021
@wg21bot wg21bot added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jan 19, 2022
@jensmaurer jensmaurer removed the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jan 19, 2022
@wg21bot wg21bot added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Feb 21, 2022
@jensmaurer jensmaurer removed the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Dec 16, 2022
@jensmaurer jensmaurer removed their assignment Dec 16, 2022
@jensmaurer
Copy link
Member Author

@CaseyCarter , please take a look.

@tkoeppe , rebased.

@tkoeppe
Copy link
Contributor

tkoeppe commented Dec 16, 2022

Thanks!

tkoeppe
tkoeppe reviewed Dec 16, 2022
View reviewed changes
source/ranges.tex Outdated Show resolved Hide resolved
CaseyCarter
CaseyCarter suggested changes Dec 16, 2022
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment •
edited

Choose a reason for hiding this comment

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

Everything I see here is good, but I think we're missing:

  • exposition-only is-vector-bool-reference in containers.tex (2 places).
  • ranges::enable_view and ranges::enable_borrowed_range partial specializations for span in containers.tex.
  • disable_sized_sentinel_for partial spec for move_iterator in iterators.tex
  • uses_allocator_v primary in memory.tex
  • Everything in meta.tex?
  • ranges::enable_borrowed_range partials for empty_view, owning_view, as_rvalue_view, reverse_view, zip_view, adjacent_view, chunk_view, slide_view in ranges.tex
  • views::istream primary in ranges.tex
  • is_bind_expression_v, is_placeholder_v, is_execution_policy_v primary in utilities.tex

@jensmaurer
Copy link
Member Author

@CaseyCarter , thanks, fixed.

CaseyCarter
CaseyCarter approved these changes Dec 16, 2022
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment •
edited

Choose a reason for hiding this comment

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

Awesome! Thanks, Jens. One little indentation nit.

source/iterators.tex Outdated Show resolved Hide resolved
@tkoeppe
Copy link
Contributor

tkoeppe commented Dec 17, 2022

All good, all done?

@jensmaurer
[lib] Drop 'inline' from 'inline constexpr' variable templates.
994d459 
Since CWG2387, constexpr variable templates have external linkage.
@jensmaurer
Copy link
Member Author

jensmaurer commented Dec 18, 2022 •
edited

Applied with commit 9ac5555

@jensmaurer jensmaurer closed this Dec 18, 2022
@jensmaurer jensmaurer deleted the c15 branch December 18, 2022 15:28
@tkoeppe
Copy link
Contributor

tkoeppe commented Dec 18, 2022

(I don't understand why this wasn't closed as "merged" when I rebase-squashed it. I thought it might have been a bug in GitHub.)

@hewillk hewillk mentioned this pull request Feb 21, 2023
Merged
JohelEGP referenced this pull request Feb 21, 2023
@jensmaurer @tkoeppe
[lib] Drop 'inline' from 'inline constexpr' variable templates.
9ac5555 
Since CWG2387, constexpr variable templates have external linkage.
@JohelEGP JohelEGP mentioned this pull request Feb 21, 2023
Merged
burblebee added a commit that referenced this pull request Feb 22, 2023
@burblebee
[ranges.syn] Remove "inline" from constexpr variable template added i…
375eb08 
…n P2164R9 as per #4625.
burblebee added a commit that referenced this pull request Feb 22, 2023
@burblebee
[refwrap.common.ref] Remove inline from constexpr template variables …
91dec5f 
…added in P2655R3 as per #4625.
tkoeppe pushed a commit that referenced this pull request Mar 7, 2023
@burblebee @tkoeppe
[ranges.syn] Remove "inline" from constexpr variable template added i…
2c0e598 
…n P2164R9 as per #4625.
tkoeppe pushed a commit that referenced this pull request Mar 11, 2023
@burblebee @tkoeppe
[refwrap.common.ref] Remove inline from constexpr template variables …
e216af2 
…added in P2655R3 as per #4625.
@frederick-vs-ja frederick-vs-ja mentioned this pull request Apr 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkoeppe tkoeppe tkoeppe left review comments

@cpplearner cpplearner cpplearner left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@timsong-cpp timsong-cpp timsong-cpp approved these changes

Assignees

@tkoeppe tkoeppe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[meta.type.synop] (and other) Repetitive 'inline constexpr'?
8 participants
@jensmaurer @zygoloid @tkoeppe @CaseyCarter @jwakely @cpplearner @timsong-cpp @wg21bot