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 bunch of test cases #183

Merged
merged 5 commits into from
Aug 19, 2022
Merged

Conversation

kant2002
Copy link
Collaborator

@kant2002 kant2002 commented Aug 8, 2022

  • Add both working, and not yet working snippets of C
  • Add support for recursive dir traversal in integration testing. Will need this for more tests to C runtime

@kant2002
Copy link
Collaborator Author

kant2002 commented Aug 9, 2022

At least 1 test should be removed after #185 lands, since I have basically duplicate test.

@ForNeVeR ForNeVeR self-requested a review August 13, 2022 19:27
Copy link
Owner

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

Also, I'm not merging this until everything's green. Have you investigated the reason for the test failures? Do you need help?

Comment on lines 4 to 8
void printf(char* s) {
// That's temporary until varargs would not be implemented.
// no formatting obviously.
puts(s);
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if it is valid for stdio.h to define the function body in a header file.

Also, this deserves a TODO comment (I'm creating an issue for each merged TODO).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. that PR is kitchen sink of different failed attempts to find some place where I can have quick win. I will try to cleanup it which is quite possible, but still it reply on earlier RPs which I send, so I waiting for them to land. I will let you know when this would be ready for your attention.

Copy link
Owner

Choose a reason for hiding this comment

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

For such cases, you may consider marking a PR as a "draft". This way, you formally let me know that it is not yet ready to merge.

An example of a draft PR is #139.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I was lazy when I create it, but cannot convert PR to draft after creation. Maybe that's some permission issue, or require some setup?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I'm not sure what the rules about drafts are, but I've found that I can convert it myself. So I did.

Let me know if you want to un-draft this, and cannot do it yourself.

Copy link
Owner

Choose a reason for hiding this comment

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

What's your opinion on this printf implementation? Should we still merge it being in a header file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would like to have Cesium.Runtime to be something minimal and provide as much implementation in C as possible. Example of functions which maybe written in C is atoi/ioa, we almost at the point where we can write them in C. Maybe that's require have something like stdlib.c which would be added automatically by compiler. That's just example, not something which I think though.

For marketing reasons, I think we should have printf even with limitation as serious as it is right now. Technically right now we don't have "Hello World".

Copy link
Owner

@ForNeVeR ForNeVeR Aug 19, 2022

Choose a reason for hiding this comment

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

My whole point is that in C, you cannot write code in headers.

If we want to write C stdlib (how do you distinguish it from Cesium.Runtime, by the way?), then we'll have to do it in .c files, and link them somehow with the user's program.

For now, this is too much. Let's migrate this to the Cesium.Runtime, and create a separate issue to discuss details of implementation parts of the standard library in C.

Comment on lines 8 to 12
public static int Abs(int value)
{
if (value == int.MinValue) return int.MinValue;
return Math.Abs(value);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Wow that's a gimmick. Let's write a unit test for the standard library, then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we can, but I mostly think that should do that as part of "acceptance testing", but in retrospection after I think a bit on this I do agree with you. That's subsystem should be tested.

@kant2002
Copy link
Collaborator Author

Abs is parsing INT_MIN. Fixed.
printf is having float point constants as part of tree, If's in progress now

@kant2002
Copy link
Collaborator Author

Ooh, abs is also requires nested #include because I add #include <math.h>

@ForNeVeR ForNeVeR marked this pull request as draft August 16, 2022 20:05
@kant2002 kant2002 force-pushed the kant/add-bunch-test-cases branch from 38ce4dd to f2093e0 Compare August 17, 2022 05:25
@kant2002
Copy link
Collaborator Author

Now it's waiting for #189 only

@ForNeVeR ForNeVeR force-pushed the kant/add-bunch-test-cases branch from f2093e0 to a79cb4c Compare August 17, 2022 17:18
- Add both working, and not yet working snippets of C
- Add support for recursive dir traversal in integration testing. Will need this for more tests to C runtime
@kant2002 kant2002 force-pushed the kant/add-bunch-test-cases branch from a79cb4c to c7ceafc Compare August 19, 2022 04:44
@kant2002 kant2002 marked this pull request as ready for review August 19, 2022 04:56
Copy link
Owner

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

Thanks!

@ForNeVeR ForNeVeR merged commit 7bbf61a into ForNeVeR:main Aug 19, 2022
@kant2002 kant2002 deleted the kant/add-bunch-test-cases branch August 19, 2022 17:54
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