Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Vm benchmark tests #299
Vm benchmark tests #299
Changes from 37 commits
b0ec357
9037b1a
d71ae44
264c184
75feb1b
17b3055
ff2ee07
e846aaa
637cbc9
fc08f80
18711b2
4b188a2
9465e89
08443b8
e10c04c
617c98f
96741c7
9d11081
a5424e2
095411c
ee75d81
8982dd7
44d77bb
4c69f1b
3c4a08a
e381ecf
d891ab3
334cf76
ab210ae
525103c
d891005
86229a4
6cf6e4f
673d6bb
49b8522
29703ea
97958f6
f5969ed
2a2ab03
3443eea
d77202f
ac43815
de78d03
4867c9c
7f691ab
0b1bd19
8b891b9
49ffce3
5e9a5e1
e57bad4
8bc7042
a763143
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think would be better to remove
setting.rs
file and defineinside lib.rs.
Now it looks redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AlI measurement results of these test are completely depend on parameters that defined in
settings.rs
file. So I think that it is better to have all adjusted parameters in separate file. Yes, there are two tests that have only one parameter but for foreign viewer it is more understandable what settings exactly have the current test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't more understandable when you have to see one 2 files instead of 1. And especially when one file has only a single line.
A 'setting' module as a separated module on the top of the file is good enough readable for a foreign viewer, I suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extern "C"
is not required and could be skipped.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not so easy question. Based on this article it can be said that
[no_mangle]
implies not only mangling absence but also participate in decision will this function be exported or not:I think that this behavior is ugly but according to 4 part
extern "C"
increases the chance that this function will exported. So i suggest to leave it on all function that should be exported.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I investigated this question and that's what I dug:
from here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it seems that
cargo fmt
adds "C". There is a separate optionforce_explicit_abi
that by default is true incargo fmt
.Also, from this link it isn't clear does ABI take part in decision of exporting function. I think that "C" may be useful later because of Rust is too young language and many things can differ in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs require a first capital letter and dot at the end.