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

STL: Down with typename? #3718

Closed
frederick-vs-ja opened this issue May 19, 2023 · 3 comments
Closed

STL: Down with typename? #3718

frederick-vs-ja opened this issue May 19, 2023 · 3 comments
Labels
enhancement Something can be improved resolved Successfully resolved without a commit

Comments

@frederick-vs-ja
Copy link
Contributor

Clang16 has implemented WG21-P0634R3, so we can omit typename in many places since C++20 mode (see WG21-P2150 for possible simplification in library). See also #3694 which drops optional typename in <mdspan>.

For pre-C++20 components, I think we can use the following conditional compilation pattern.

#if _HAS_CXX20
#define _OPT_TYPENAME
#else // ^^^ _HAS_CXX20 / !_HAS_CXX20 vvv
#define _OPT_TYPENAME typename
#endif // ^^^ !_HAS_CXX20 ^^^

I'm not sure whether this would

  • make the code easier to read, and/or
  • improve compiler throughput,

or the way around.

@CaseyCarter CaseyCarter added enhancement Something can be improved decision needed We need to choose something before working on this labels May 19, 2023
@CaseyCarter
Copy link
Member

I'm happy to omit typename where it's no longer required in new code.

I think using a macro for this in pre-C++20 code only obfuscates the code. I can't imagine the time spent parsing the keyword is significant enough to offset the cost of macro replacement; I think we'd either improve or pessimize throughput by a negligible amount.

I also feel like it's not worth our time to prepare and review PRs that only remove typename from existing code. The tiny improvement to readability doesn't seem worth the investment of time we could spend elsewhere. Of course this is open source, and people work on what they want to, so 🤷.

Disclaimer: It's hard for me to be objective, I think "Down with typename" was a mistake that complicates the language by adding exceptions to what was a simple universal rule. I've marked this issue "decision needed" incase anyone would like to disagree with my above opinions. (Opinions on what we should do in the STL, that is, not my dislike of "Down with typename". That's a #non-stl Discord discussion topic at best.)

@StephanTLavavej StephanTLavavej removed the decision needed We need to choose something before working on this label May 24, 2023
@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting and we concluded that:

  • C++20-and-above code should omit typename when it's allowed
    • We want to avoid "chaos" where code follows inconsistent conventions
      • If there is a transition period where the old and new conventions co-exist, at a minimum we should have consistency within each file - e.g. adding <mdspan> following the omit-typename convention is fine, but if code is being added to an existing header, it should follow that header's current convention.
    • We usually try to avoid disrupting test code with cleanups that might potentially change coverage, but the diff here seems extremely easy to review and low risk (just dropping typename)
  • We are uniformly and stridently opposed to any macro-based approach; for C++14/17 code we should simply say typename as we're doing today
  • The maintainers are open to reviewing PRs to convert the codebase to this new convention, but likely won't have time to create such PRs themselves
  • Please avoid unrelated drive-by fixes in large mechanical PRs as they increase reviewing difficulty and risk
  • A single large PR to convert the entire codebase could be feasible; it could also be structured as a series of commits that update each kind of context (e.g. casts, defining-type-ids, etc.).

@StephanTLavavej
Copy link
Member

I believe that this is done, with my #4191 performing a final audit of both the product and test code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved resolved Successfully resolved without a commit
Projects
None yet
Development

No branches or pull requests

3 participants