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

Remove swap parameter and related code #706

Closed
wants to merge 2 commits into from

Conversation

stweil
Copy link
Contributor

@stweil stweil commented Feb 6, 2017

Remove all code for auto detection of endianness used in data files.
This prepares the switch to data files with fixed endianness.

Signed-off-by: Stefan Weil sw@weilnetz.de

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Remove all code for auto detection of endianness used in data files.
This prepares the switch to data files with fixed endianness.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
@stweil
Copy link
Contributor Author

stweil commented Feb 6, 2017

@theraysmith, this PR is ready to get pulled as soon as there is a decision to switch to fixed endianness (little endian) for the training data files.

if (swap) {
ReverseN(&size1, sizeof(size1));
ReverseN(&size2, sizeof(size2));
}
Resize(size1, size2, empty_);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a malloc failure and a crash right here if you don't reverse the size to be allocated!

@theraysmith
Copy link
Contributor

Rejected. I don't see how this can possibly work.
See comment on matrix.h.

@theraysmith theraysmith closed this Feb 7, 2017
@stweil
Copy link
Contributor Author

stweil commented Feb 7, 2017

I'm afraid there is a misunderstanding. This patch does not implement big endian support. It removes the current effort to support big endian as a first step for a different implementation which I think is more promising. PR #703 mixes removing the current implementation and implementing a new one and should be rebased on PR #706.

@stweil stweil mentioned this pull request Feb 7, 2017
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.

2 participants