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

change fasta and fasta-redux benchmark to Veedrac's implementation #28623

Merged
merged 2 commits into from
Sep 29, 2015
Merged

change fasta and fasta-redux benchmark to Veedrac's implementation #28623

merged 2 commits into from
Sep 29, 2015

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Sep 24, 2015

I just removed the num_cpus dependency (because we don't want that in there), using 4 threads instead.

I should add that Veedrac asked me to submit this here in his name.

@rust-highfive
Copy link
Collaborator

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@llogiq llogiq changed the title change fasta benchmark to Veedrac's implementation change fasta and fasta-redux benchmark to Veedrac's implementation Sep 24, 2015
@alexcrichton
Copy link
Member

r? @brson

Not sure if there's license things here we should worry about

@rust-highfive rust-highfive assigned brson and unassigned pcwalton Sep 24, 2015
@llogiq
Copy link
Contributor Author

llogiq commented Sep 24, 2015

@Veedrac may have something to say about that.

@Veedrac
Copy link
Contributor

Veedrac commented Sep 24, 2015

Thanks for submitting this.

I'm not totally sure what licensing issue is being mentioned. I wouldn't be concerned if it was accepted as-is, though.

@llogiq
Copy link
Contributor Author

llogiq commented Sep 25, 2015

As far as I'm concerned, the implementation makes use of algorithms from both wikipedia and other benchmark sources (in different languages). The used algorithms themselves aren't subject to any patents I know of, and AFAIK Veedrac didn't make use of other sources that would have require him to submit to a license. I've prepended the usual headers to both files (and I think Veedrac's comment clarifies his agreement to allowing rust-lang to use his code).

@alexcrichton What other license things should we look out for?

@alexcrichton
Copy link
Member

I have no specific concerns, I simply do not know what to even look for. @brson is our resident license expert so I'm deferring to him. Everything is probably fine, I just don't want to have to patch anything up after-the-fact.

@llogiq
Copy link
Contributor Author

llogiq commented Sep 28, 2015

Good to know. So @brson, do we need to clarify something?

@brson
Copy link
Contributor

brson commented Sep 29, 2015

@bors r+ lgtm

@bors
Copy link
Contributor

bors commented Sep 29, 2015

📌 Commit 12d990d has been approved by brson

@bors
Copy link
Contributor

bors commented Sep 29, 2015

⌛ Testing commit 12d990d with merge 19fe7b6...

bors added a commit that referenced this pull request Sep 29, 2015
I just removed the num_cpus dependency (because we don't want that in there), using 4 threads instead.

I should add that Veedrac asked me to submit this here in his name.
@bors bors merged commit 12d990d into rust-lang:master Sep 29, 2015
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.

7 participants