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

time.h: time() and ctime() #253

Merged
merged 4 commits into from
Oct 23, 2017
Merged

time.h: time() and ctime() #253

merged 4 commits into from
Oct 23, 2017

Conversation

yulvil
Copy link
Contributor

@yulvil yulvil commented Oct 19, 2017

This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 19, 2017

Codecov Report

Merging #253 into master will increase coverage by 0.82%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
+ Coverage   80.79%   81.61%   +0.82%     
==========================================
  Files         134      135       +1     
  Lines        5336     5525     +189     
==========================================
+ Hits         4311     4509     +198     
+ Misses        835      825      -10     
- Partials      190      191       +1
Impacted Files Coverage Δ
types/resolve.go 83.87% <ø> (+3.22%) ⬆️
program/function_definition.go 100% <ø> (ø) ⬆️
noarch/time.go 93.75% <93.75%> (ø)
transpiler/binary.go 68.78% <0%> (-2.36%) ⬇️
transpiler/enum.go 95.83% <0%> (-1.76%) ⬇️
program/program.go 70.21% <0%> (+0.64%) ⬆️
ast/position.go 82.33% <0%> (+0.75%) ⬆️
transpiler/operators.go 85.11% <0%> (+1.19%) ⬆️
ast/implicit_cast_expr.go 91.3% <0%> (+1.83%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b418fd...9d427af. Read the comment docs.

@elliotchance
Copy link
Owner

Reviewed 4 of 4 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


tests/time.c, line 18 at r2 (raw file):

    now = time(&tloc);
    is_true(now == tloc);

Please use is_eq(now, tloc) instead. This will show the value of both if it fails.


tests/time.c, line 20 at r2 (raw file):

    is_true(now == tloc);

    pass("%s", "time");

This is unesseary because there is an assertions above. However, it would be a good idea to also check: is_not_eq(now, 0)


tests/time.c, line 30 at r2 (raw file):

    time_t now = 946684798;
    s = ctime(&now);
    printf("%s", s);

Avoid printf as it will break the TAP output. You can use is_streq: https://github.com/elliotchance/c2go/blob/master/tests/tests.h#L198


tests/time.c, line 32 at r2 (raw file):

    printf("%s", s);

    pass("%s", "ctime");

Like the previous pass this doesn't actually check anything and can be removed.


Comments from Reviewable

@elliotchance
Copy link
Owner

elliotchance commented Oct 19, 2017

Thanks for putting in the PR. I started to review it as you were still making changes :)

Please respond to the discussions on Reviewable (purple button at the top).

@yulvil
Copy link
Contributor Author

yulvil commented Oct 20, 2017

Review status: 2 of 4 files reviewed at latest revision, 4 unresolved discussions.


tests/time.c, line 18 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Please use is_eq(now, tloc) instead. This will show the value of both if it fails.

Done.


tests/time.c, line 20 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

This is unesseary because there is an assertions above. However, it would be a good idea to also check: is_not_eq(now, 0)

Done.


tests/time.c, line 30 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Avoid printf as it will break the TAP output. You can use is_streq: https://github.com/elliotchance/c2go/blob/master/tests/tests.h#L198

Done.


tests/time.c, line 32 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Like the previous pass this doesn't actually check anything and can be removed.

Done.


Comments from Reviewable

@elliotchance
Copy link
Owner

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@yulvil
Copy link
Contributor Author

yulvil commented Oct 21, 2017

@elliotchance Thank you for reviewing. I am having a hard time troubleshooting the OS X error; I do not have a machine at hand.

I tried to compile similar code in Travis. The build succeeded.

Is it possible to add debug for the Travis/OS X build? Any help appreciated.

@elliotchance
Copy link
Owner

When I run it on my Mac:

Elliots-MacBook-Pro-3:c2go elliot$ clang tests/time.c
Elliots-MacBook-Pro-3:c2go elliot$ ./a.out
1..6
# time
1 ok - now != 0
2 ok - now != 0
3 ok - now == tloc
# ctime
Segmentation fault: 11

It looks like ctime expects that the parameter not be NULL.

When I removed https://github.com/elliotchance/c2go/pull/253/files#diff-c3bb064a4d264cc6f2d46ab300725b61R25 I did not get the segfault.

Even the same compiler on different operating systems can give different results. You can remove line 25 and 26 and your build should pass.

@elliotchance
Copy link
Owner

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@elliotchance elliotchance merged commit aaae775 into elliotchance:master Oct 23, 2017
@elliotchance
Copy link
Owner

Awesome - thanks for the PR! 👍

@yulvil yulvil deleted the timeh branch October 23, 2017 06:30
@yulvil
Copy link
Contributor Author

yulvil commented Oct 23, 2017

@elliotchance Thank you for merging!

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