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

Adjustments to std.system. #186

Merged
merged 2 commits into from
Sep 4, 2011
Merged

Adjustments to std.system. #186

merged 2 commits into from
Sep 4, 2011

Conversation

jmdavis
Copy link
Member

@jmdavis jmdavis commented Aug 13, 2011

I went in to fix the enum values so that they followed the Phobos naming conventions and ended up trying to clean it up a bit and make it more complete. Honestly though, I'm not sure how much of std.system is truly useful. I'm very tempted to remove the os variable at the bottom as well as os_major and os_minor - none of which have been documented. There's no way that os will ever be correct in the general case. It might be able to be made to work on Windows but not on any Posix system - especially if we go beyond the basic Linux, OSX, FreeBSD to be more specific (and OS has had RedHatLinux in it, so it's obviously intended to be more specific on more than just Windows). So, honestly, I'd be tempted to axe the OS enum entirely. Family serves about the same purpose as Endian - it gives runtime versions of the version values - so they're both of some value, but OS seems far less useful to me. I did expand it to be more thorough, but I'd honestly rather outright remove it. Its main purpose seems to be to be used in conjunction with the os variable to indicate which OS you're currently running on, and I just don't see how that is generally feasible.

static assert(0);
}
/// The family of OS that the program was compiled for.
version(StdDdoc) Family family;
Copy link
Contributor

Choose a reason for hiding this comment

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

Family family; is equals to Family family = Family.win32;. Is it expected behaviour? Is so, why win32 is set to 1 insted of default 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Notice the StdDdoc. That declaration is only for the documentation. Its value doesn't matter. It'll never actually be used in code. version(StdDdoc) is used so that it'll show up no matter which system is used to generate the documentation without having to duplicate the documentation for every version block.

As to why win32 is 1, I don't know. I didn't change its value. It was that originally.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so, as it shouldn't be used, variable cannot be read at compile time error is good.

Copy link
Member

Choose a reason for hiding this comment

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

@denis-sh: No, you are getting this wrong – the declaration is only used if the Phobos documentation is built (the StdDdoc is set). During normal compilation, it has no effect.

@jmdavis: Are you sure the extra DDoc version is actually required? As far as I know, a DDoc comment always affects the next declaration, and as versioned out ones are just skipped…

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm. I'm pretty darn sure that when you have

/// ddoc comment
version(Windows)    code
else version(linux)    code

you don't get the documentation on Linux, but it could be that the code that I dealt with before which had that problem before was actually

version(Windows)
{
    ///ddoc comment
    code
}
else version(linux) code

At this point, other places in Phobos (e.g. std.file) use this scheme when they have to version code like this. I'd have to experiment with it though to be 100% sure that the first example doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

@jmdavis: I just checked and just placing the documentation outside the version block seems to work for me – are there any cases where it breaks? I'm aware that this scheme is used in other places, but I think we should avoid StdDdoc as much as possible.

@jmdavis
Copy link
Member Author

jmdavis commented Aug 14, 2011

Okay. In light of the issues with OS, os, major_os, and minor_os, I think that they should just be deprecated. They differ too much from OS to OS to be handled generically in any kind of reasonable matter. So, I redid my changes to reflect that, and I confirm that version(StdDdoc) is not actually necessary in this case, so I removed it. I also improved the documentation a bit to make it clearer that version should be used instead of these enums if you're doing conditional compilation.

@andralex
Copy link
Member

andralex commented Sep 4, 2011

I think the main question here is about how much code this will break.

@jmdavis
Copy link
Member Author

jmdavis commented Sep 4, 2011

Very little, I would think, since most of this module has been completely undocumented. Changing Endian may break a fair bit of code, though in most cases that I've seen Endian use (e.g. in a static if), it really should have used a version block. The question is whether fixing the enums to follow the correct naming conventions is worth the potential code breakage. I would expect the usage of even Endian to be fairly rare, but I don't know.

@andralex
Copy link
Member

andralex commented Sep 4, 2011

Sounds good, thanks. Please rebase?

@jmdavis
Copy link
Member Author

jmdavis commented Sep 4, 2011

Okay. I redid it all as one commit on the latest master. I didn't add the duplicate values for Endian though.

@andralex
Copy link
Member

andralex commented Sep 4, 2011

Still can't merge for some reason.

Conflicts:
	std/format.d
@jmdavis
Copy link
Member Author

jmdavis commented Sep 4, 2011

This pull request makes a couple of small changes to std.format (since it uses Endian in a couple of places), and another pull request with changes to std.format was merged in since I updated the pull request.

andralex added a commit that referenced this pull request Sep 4, 2011
Adjustments to std.system.
@andralex andralex merged commit 0764204 into dlang:master Sep 4, 2011
kuettler pushed a commit to kuettler/phobos that referenced this pull request Feb 6, 2018
kuettler pushed a commit to kuettler/phobos that referenced this pull request Feb 6, 2018
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