Skip to content

fix nlopt_srand for 32bit machines#9

Merged
dataPulverizer merged 2 commits intoDlangScience:masterfrom
jmh530:featureA
Mar 14, 2016
Merged

fix nlopt_srand for 32bit machines#9
dataPulverizer merged 2 commits intoDlangScience:masterfrom
jmh530:featureA

Conversation

@jmh530
Copy link
Copy Markdown
Contributor

@jmh530 jmh530 commented Mar 11, 2016

The input type on nlopt_srand needs to account for the difference in 32bit/64bit
machines.

The original C signature of nlopt_srand is
NLOPT_EXTERN(void) nlopt_srand(unsigned long seed);

According to http://wiki.dlang.org/D_binding_for_C
unsigned long only is converted to ulong only on 64bit, but on 32bit it needs
to be uint.

I'm not completely confident I'm using the correct version() blocks.

@John-Colvin
Copy link
Copy Markdown
Member

you should use core.stdc.config.c_ulong. core.stdc.config isn't currently documented on the website, but that's an oversight not a meaningful decision. See https://github.com/D-Programming-Language/druntime/blob/master/src/core/stdc/config.d

@jmh530
Copy link
Copy Markdown
Contributor Author

jmh530 commented Mar 11, 2016

If I'm reading this correctly, it looks like c_ulong gives uint on Windows always and then switches on Posix depending on the architecture.

@John-Colvin
Copy link
Copy Markdown
Member

Correct, but the point of core.stdc.config is that you don't have to
think about any of that, you just use c_ulong and then it's right no
matter what architecture you're on.
On 11 Mar 2016 2:35 pm, "jmh530" notifications@github.com wrote:

If I'm reading this correctly, it looks like c_ulong gives uint on Windows
always and then switches on Posix depending on the architecture.


Reply to this email directly or view it on GitHub
#9 (comment).

@jmh530
Copy link
Copy Markdown
Contributor Author

jmh530 commented Mar 11, 2016

Ok. That's great and I will change when I have a chance. It does look like
my link references core.stdc.config.c_ulong. Nevertheless, I find it a
little disconcerting that the Windows behavior (unsigned long is uint
always) is not mentioned anywhere. I wouldn't have known that without
looking at c_ulong's code.

On Fri, Mar 11, 2016 at 10:04 AM, John Colvin notifications@github.com
wrote:

Correct, but the point of core.stdc.config is that you don't have to
think about any of that, you just use c_ulong and then it's right no
matter what architecture you're on.
On 11 Mar 2016 2:35 pm, "jmh530" notifications@github.com wrote:

If I'm reading this correctly, it looks like c_ulong gives uint on
Windows
always and then switches on Posix depending on the architecture.


Reply to this email directly or view it on GitHub
#9 (comment).


Reply to this email directly or view it on GitHub
#9 (comment).

@John-Colvin
Copy link
Copy Markdown
Member

Presumably if someone is using c_ulong then they're interfacing to C, in
which case of course it can be a different size between any two targets,
because that's how C works. Or am I misunderstanding your complaint?
On 11 Mar 2016 3:15 pm, "jmh530" notifications@github.com wrote:

Ok. That's great and I will change when I have a chance. It does look like
my link references core.stdc.config.c_ulong. Nevertheless, I find it a
little disconcerting that the Windows behavior (unsigned long is uint
always) is not mentioned anywhere. I wouldn't have known that without
looking at c_ulong's code.

On Fri, Mar 11, 2016 at 10:04 AM, John Colvin notifications@github.com
wrote:

Correct, but the point of core.stdc.config is that you don't have to
think about any of that, you just use c_ulong and then it's right no
matter what architecture you're on.
On 11 Mar 2016 2:35 pm, "jmh530" notifications@github.com wrote:

If I'm reading this correctly, it looks like c_ulong gives uint on
Windows
always and then switches on Posix depending on the architecture.


Reply to this email directly or view it on GitHub
#9 (comment).


Reply to this email directly or view it on GitHub
#9 (comment).


Reply to this email directly or view it on GitHub
#9 (comment).

@jmh530
Copy link
Copy Markdown
Contributor Author

jmh530 commented Mar 11, 2016

My point is that all the D documentation on interfacing with C says that the behavior changes between 32bit and 64bit. It does not say that this behavior does not apply in Windows.

On Mar 11, 2016, at 10:23 AM, John Colvin notifications@github.com wrote:

Presumably if someone is using c_ulong then they're interfacing to C, in
which case of course it can be a different size between any two targets,
because that's how C works. Or am I misunderstanding your complaint?
On 11 Mar 2016 3:15 pm, "jmh530" notifications@github.com wrote:

Ok. That's great and I will change when I have a chance. It does look like
my link references core.stdc.config.c_ulong. Nevertheless, I find it a
little disconcerting that the Windows behavior (unsigned long is uint
always) is not mentioned anywhere. I wouldn't have known that without
looking at c_ulong's code.

On Fri, Mar 11, 2016 at 10:04 AM, John Colvin notifications@github.com
wrote:

Correct, but the point of core.stdc.config is that you don't have to
think about any of that, you just use c_ulong and then it's right no
matter what architecture you're on.
On 11 Mar 2016 2:35 pm, "jmh530" notifications@github.com wrote:

If I'm reading this correctly, it looks like c_ulong gives uint on
Windows
always and then switches on Posix depending on the architecture.


Reply to this email directly or view it on GitHub
#9 (comment).


Reply to this email directly or view it on GitHub
#9 (comment).


Reply to this email directly or view it on GitHub
#9 (comment).


Reply to this email directly or view it on GitHub.

@John-Colvin
Copy link
Copy Markdown
Member

Fair point. If you have a clearer way to express the situation in the dlang
docs then it's quite easy to make the relevant edit and submit a pull
request entirely via the github web interface. Feel free to drop me an
email if it doesn't work out as expected.
On 11 Mar 2016 6:17 pm, "jmh530" notifications@github.com wrote:

My point is that all the D documentation on interfacing with C says that
the behavior changes between 32bit and 64bit. It does not say that this
behavior does not apply in Windows.

On Mar 11, 2016, at 10:23 AM, John Colvin notifications@github.com
wrote:

Presumably if someone is using c_ulong then they're interfacing to C, in
which case of course it can be a different size between any two targets,
because that's how C works. Or am I misunderstanding your complaint?
On 11 Mar 2016 3:15 pm, "jmh530" notifications@github.com wrote:

Ok. That's great and I will change when I have a chance. It does look
like
my link references core.stdc.config.c_ulong. Nevertheless, I find it a
little disconcerting that the Windows behavior (unsigned long is uint
always) is not mentioned anywhere. I wouldn't have known that without
looking at c_ulong's code.

On Fri, Mar 11, 2016 at 10:04 AM, John Colvin notifications@github.com
wrote:

Correct, but the point of core.stdc.config is that you don't have to
think about any of that, you just use c_ulong and then it's right no
matter what architecture you're on.
On 11 Mar 2016 2:35 pm, "jmh530" notifications@github.com wrote:

If I'm reading this correctly, it looks like c_ulong gives uint on
Windows
always and then switches on Posix depending on the architecture.


Reply to this email directly or view it on GitHub
<#9 (comment)
.


Reply to this email directly or view it on GitHub
#9 (comment).


Reply to this email directly or view it on GitHub
#9 (comment).


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#9 (comment).

@dataPulverizer
Copy link
Copy Markdown
Collaborator

Should the version flags not be added to the build script or does dub somehow do this automatically?

@jmh530
Copy link
Copy Markdown
Contributor Author

jmh530 commented Mar 14, 2016

Dub can detect what OS you're on, but if you want 32bit or 64bit, then you have to submit --arch at the command line.

There probably is scope to adjust the libs line in the dub.json to be libs-posix. When I use this on Windows, I have to adjust that.

@John-Colvin
Copy link
Copy Markdown
Member

dmd sets whatever builtin flags are relevant for the chosen target.

@dataPulverizer
Copy link
Copy Markdown
Collaborator

That's good, I'll merge now then

dataPulverizer added a commit that referenced this pull request Mar 14, 2016
fix nlopt_srand for 32bit machines
@dataPulverizer dataPulverizer merged commit 355195f into DlangScience:master Mar 14, 2016
@John-Colvin
Copy link
Copy Markdown
Member

@jmh530 Note for the future: instead of having one commit that does something the wrong way and then another immediately following it to fix it, you can make your changes and then git add <changed file> then git commit --amend to amend the previous commit, or if you've already commited then you can either git reset HEAD~1 to "un-commit" then do the above or alternatively git rebase -i HEAD~2 and the follow the instructions to squash one of the commits in to the other.

You will have to do git push --force in both cases because you are effectively rewriting history, so it's a very bad idea to do on important public branches that other people are likely to have checked out and be using, but OK for a pull request. You might find this chart useful for reference/understanding: https://github.com/nickdesaulniers/git-dot/blob/master/staging.dot.svg

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.

3 participants