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

Add in_set definition #26

Closed
wants to merge 1 commit into from
Closed

Add in_set definition #26

wants to merge 1 commit into from

Conversation

ekaitz-zarraga
Copy link
Contributor

With this in_set the hex0.c can be compiled and used to test if new implementations of hex0 are working as expected.

Comment on lines 32 to 45
int in_set(int c, char* s){
for(int i=0;;i++){
char a = s[i];
if(a=='\0'){
return FALSE;
}
if(a == c){
return TRUE;
}
}
}
Copy link
Collaborator

@stikonas stikonas May 21, 2021

Choose a reason for hiding this comment

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

Can you keep coding style same as in other parts of the file?
Mostly braces, add spaces around ==.

This addition makes the file compilable so it can be used to test new implementations of hex0 against it.
@ekaitz-zarraga
Copy link
Contributor Author

@stikonas I reviewed the style, hope it's ok now.
Thanks for the comment.

@schierlm
Copy link
Contributor

I noticed that not only hex0 but also hex1 and hex2 used the same prototype.

Now that stage0-posix was updated to include M2libc, I guess it might be a better idea to instead update the high-level prototype to M2libc and include a Makefile, so that it is more obvious that you need to link with bootstrappable.c

See #27

@oriansj
Copy link
Owner

oriansj commented May 23, 2021

Thank you for bringing this issue to our attention.
With the M2libc upgrade this was accidentally dropped on the floor.
But now that it is complete, using the M2libc definition instead of code duplication is a cleaner and more maintainable solution long term.

@oriansj oriansj closed this May 23, 2021
@ekaitz-zarraga
Copy link
Contributor Author

In that case, I must suggest to move the hex function out too, because it appears in hex0 and hex1 and probably others do too.

:)

@oriansj
Copy link
Owner

oriansj commented May 26, 2021

Absolutely fair ekaitz-zarraga. and if you wish to make a pull request for that change, I certainly would be more than happy to merge that ^_^

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.

None yet

4 participants