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

Conversation

eskimor
Copy link

@eskimor eskimor commented Jun 30, 2012

Hi!

This little patch fixes afore mentioned bug, that the stat struct is not correct on Linux systems.

It also introduces a unit test for testing the correctness. In addition I added the statvfs struct for Linux systems, also with unit test.

@MartinNowak
Copy link
Member

Thanks for contributing to druntime.

  • Let's split this into two pull requests (bugfix and statvfs).
  • Headers in core.sys.posix should not contain platform specific extensions (those would go into core.sys.OS) and should have four sections (Open Group specification, linux, OSX and FreeBSD). Sean wrote something about this approach here.
  • Please avoid unrelated whitespace/style changes and tabs.

@eskimor
Copy link
Author

eskimor commented Jul 7, 2012

Hi!
Thanks a lot for this very fast reply. I was just moving the wordsize stuff to core.sys.linux and started looking on the other stuff in core.sys.posix.config.d. I added some Linux specific stuff there, but the whole file is actually just Linux specific. What should I put there and what should I put in core.sys.linux?

Tanks!

Best regards,

Robert

@ghost ghost assigned MartinNowak Jul 8, 2012
@andralex
Copy link
Member

andralex commented Jul 8, 2012

@dawgfoto I'll assign this to you - please let @eskimor know how to proceed. Thanks!

@andralex
Copy link
Member

ping on this

@MartinNowak
Copy link
Member

We try to keep this headers standard compliant. I think what we're currently porting is roughly SUSv3 (POSIX.1-2001 + XSI) which you'll get if _XOPEN_SOURCE is defined to 600 while compiling. We'd have to assess what's necessary for switching to SUSv4 (POSIX.1-2008 + XSI).

@complexmath do you have any further information?

http://pubs.opengroup.org/onlinepubs/009604499/nfindex.html

This means __USE_XOPEN2K8, _BSD_SOURCE, _SVID_SOURCE, __USE_GNU,
__USE_MISC should be false in core.sys.posix.

http://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html

If __WORDSIZE is widely used in linux headers then it makes sense to declare it here. We should probably comment that these are global glibc/linux defines.

I intend to start a test suite for druntime and would like to see your unit test in there, but I'm not so positive about ad-hoc unittest additions to the makefile.

Please watch out to avoid tabs and trailing whitespace and use K&R style braces if it's the predominant style. There is also a small D style guide.


version(Alpha) {
enum __WORDSIZE=64;
}
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to use D_LP64 instead?

Copy link
Author

Choose a reason for hiding this comment

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

Well at least on x86 it seems to be the same. Thanks! I'll use it instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

__WORDSIZE should be determinable from D_LP64 on all relevant platforms these days.

@eskimor
Copy link
Author

eskimor commented Jul 22, 2012

Thanks very lot for your comments, I will examine your comments and proposals thoroughly. I am sorry for the delay as I was on holiday the last two weeks, but I just came back and I am motivated :-)

unittests: absolutely. It is also pretty redundant work to add struct unit tests for every struct, unfortunately C has no mixins. But a code generator would do. If desired, I could write a pair of D mixin and C code generator for this purpose. Maybe having a directory for auto generated C unit test helper functions, which are automatically included in the unittest build?

Should I remove the unit tests for now from the pull request? Or leave them until we have a better solution?

Thanks for the style guidelines, I will adhere to them in the next update.

@MartinNowak
Copy link
Member

I am sorry for the delay

No need to hurry.

Should I remove the unit tests for now from the pull request?

I'd opt for leaving them out for now. I want to start a test suite like dmd and there would be plenty of space for such tests, so it's probably even OK to write and extend them manually.

enum __WORDSIZE=32;
}

version(MIPS6) {
Copy link
Contributor

Choose a reason for hiding this comment

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

MIPS64

Copy link
Author

Choose a reason for hiding this comment

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

thanks!

@andralex
Copy link
Member

Alrighty, waiting for one week on this.

eskimor added 3 commits July 24, 2012 20:44
Avoided unrelated whitspace changes.
Removed unit tests, as they are being replaced by a more general suite,
according to Martin Nowak.

Work in progress. Not yet tested.
Added version(posix) section in config, currently not used.
Fixed whitespace in config.
As suggested by Martin Nowak.
@eskimor
Copy link
Author

eskimor commented Jul 24, 2012

Just updated request. It now only contains the needed changes to make struct stat work. Although the unit tests are now no longer included in the request, I tested the commit with them and they worked with both MODEL=32 and MODEL=64. In addition I tested my deimos-fuse wrapper with it and it seems to work too.
I think I incorporated all suggestions so far, please have a look. :-)

@MartinNowak MartinNowak merged commit e62b8d8 into dlang:master Jul 26, 2012
@MartinNowak
Copy link
Member

I merged this with some smaller adjustments ccaa0ef, thanks.

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

Successfully merging this pull request may close these issues.

4 participants