Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Removing __mbstate_t union definition from core/stdc/stdio.d #2473

Merged
merged 3 commits into from
Jan 23, 2019
Merged

Removing __mbstate_t union definition from core/stdc/stdio.d #2473

merged 3 commits into from
Jan 23, 2019

Conversation

rmboggs
Copy link
Contributor

@rmboggs rmboggs commented Jan 22, 2019

Both NetBSD and OpenBSD have _mbstate_t union definitions in both stdio.d and wchar.d files in src/core/std. Since _mbstate_t should be defined in wchar.d and __mbstate_t isn't used by Net/OpenBSD in stdio.d anyway, just remove the defs from stdio.d and it should be ok.

Please let me know if anything else needs to be modified.

…BSD and OpenBSD versions as they are already defined in core/stdc/wchar_.d and it is not used for any structure definition in stdio.d for Net/OpenBSD.
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @rmboggs! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + druntime#2473"

@PetarKirov
Copy link
Member

PetarKirov commented Jan 22, 2019

Thanks! Can you move the FreeBSD and DragonFlyBSD definitions to wchar_.d as well?

@ibuclaw
Copy link
Member

ibuclaw commented Jan 22, 2019

LGTM, just noted comments to address.

@rmboggs
Copy link
Contributor Author

rmboggs commented Jan 22, 2019

I was wondering about the other BSDs, actually. The only reason why I left them as is was because I didn't know what else relied on mbstate being in stdio.d, for FreeBSD at least since it is a supported platform.

If you guys feel that such a change is safe, I'll do it. I'm just being cautious.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 22, 2019

Perhaps it should go in its own, but given a choice between the two, wchar_.d looks to be the better place to have the definition.

In stdio.d, just be sure to have import core.stdc.wchar_ : mbstate_t for the BSDs that use it.

@PetarKirov
Copy link
Member

PetarKirov commented Jan 22, 2019

As far as I could see, in FreeBSD's libc implementation __mbstate_t is definited in <sys/_typses.h> and typedef-ed to mbstate_t in <wchar.h> More importantly, the C standard says that mbstate_t is definited by <wchar.h>, so the definition should go there (in core.sys.stdc.wchar_.d). And, as far as core.sys.stdc.stdio is concerned, it should only privately import core.stdc.wchar_ : mbstate_t and no users of stdio should assume that mbstate_t is exposed from there.

@rmboggs
Copy link
Contributor Author

rmboggs commented Jan 23, 2019

Ok, makes sense. I'll push an update as soon as I can. Thanks.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 23, 2019

The remaining odd one out looks to be Solaris, but I'm OK with handling that separately.

@rmboggs
Copy link
Contributor Author

rmboggs commented Jan 23, 2019

Ok, well. All the other BSDS are updated accordingly now. Please take a good look at the FreeBSD and DragonFlyBSD updates as I can't test those at home currently.

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

Everything looks good to me, nice work!

@dlang-bot dlang-bot merged commit 3d4d437 into dlang:master Jan 23, 2019
@rmboggs
Copy link
Contributor Author

rmboggs commented Jan 23, 2019

Thanks, glad to help.

Do you still need me to refactor mbstate_t for solaris? Looking at the code, I thought that one was fine as-is.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 23, 2019

#2474

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants