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

Fix Issue 24215 - isBasicType!Enum should be false #8838

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

pbackus
Copy link
Contributor

@pbackus pbackus commented Nov 1, 2023

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @pbackus! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
24215 normal std.traits.isBasicType!Enum should be false

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8838"

@pbackus
Copy link
Contributor Author

pbackus commented Nov 1, 2023

Dub build failure is caused by this change somehow; time to investigate...

dlang-bot pushed a commit to dlang/dub that referenced this pull request Nov 1, 2023
@thewilsonator
Copy link
Contributor

Dependant PR merged.

@dlang-bot dlang-bot merged commit 98326c4 into dlang:master Nov 2, 2023
14 checks passed
@dkorpel
Copy link
Contributor

dkorpel commented Nov 2, 2023

Considering it broke a project on buildkite, it probably warrants a change log entry

pbackus added a commit to pbackus/phobos that referenced this pull request Nov 2, 2023
dlang-bot pushed a commit that referenced this pull request Nov 2, 2023
@n8sh
Copy link
Member

n8sh commented Nov 4, 2023

Hang on a minute! The way std.traits classifies types doesn't always line up with the language specification and nothing says it has to. For example as far as the D language is concerned char and bool are unsigned integer types but std.traits.isUnsigned and std.traits.isIntegral present a view of the world that draws a distinction between characters, boolean values, and numbers. Breaking changes to std.traits are particularly bad given how foundational it is. Even D programmers who use little else of Phobos rely on it.

Is there some common coding pattern using isBasicType that runs into problems if the programmer doesn't explicitly consider enums and exclude them? That would be a decent reason to consider changing it.

@pbackus
Copy link
Contributor Author

pbackus commented Nov 4, 2023

I wanted this version of isBasicType for my safe allocators project, but I ended up finding a different solution to the same problem. So at this point it's just a UX issue.

If consensus is that this is too risky, I'm fine with reverting it. We can revisit for Phobos v2, if and when that ever happens.

@atilaneves, thoughts?

@atilaneves
Copy link
Contributor

If there are issues and you have a workaround, I say it's better/easier to revert.

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

Successfully merging this pull request may close these issues.

6 participants