Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor time retrieval functions #212

Merged

Conversation

visitorckw
Copy link
Collaborator

This commit refactors the time retrieval functions in utils.c to reduce code duplication and improve maintainability. The previous rv_gettimeofday and rv_clock_gettime functions contained similar logic, which has now been extracted into a helper function named get_time_info. This not only makes the code more clear and concise but also enhances reusability.

Summary of changes:

  • Created the get_time_info function for retrieving time information, reducing code duplication.
  • Called the get_time_info function in the rv_gettimeofday and rv_clock_gettime functions.

This modification does not affect the functionality of rv32emu but improves code readability and maintainability.

@visitorckw visitorckw force-pushed the refactor/time-retrieval-functions branch from 52300cd to 465948b Compare September 9, 2023 02:25
src/utils.c Outdated Show resolved Hide resolved
src/utils.c Outdated Show resolved Hide resolved
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Shorten the git commit message a bit.

@visitorckw visitorckw force-pushed the refactor/time-retrieval-functions branch from 465948b to 7c57a6c Compare September 9, 2023 03:46
tp->tv_sec = tv_sec;
tp->tv_nsec = tv_msec;
tp->tv_nsec = tv_usec / 1000; /* Transfer to microseconds */
Copy link
Contributor

Choose a reason for hiding this comment

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

The statement appears somewhat unusual. When I initially worked on incorporating newlib support, I encountered some confusion regarding the time scale and units. Could you provide clarification regarding the implementation and expectations related to newlib support?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason for this statement is that when I was reviewing commit 686554c, I noticed that Newlib was manipulating time with millisecond resolution, even though clock_gettime expects nanoseconds in the timespec struct. However, I did not investigate this further at the time. Perhaps I can take a closer look to understand the underlying reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add TODO/FIXME comments and submit another pull request(s) when you are ready to resolve the newlib issue.

Copy link
Collaborator Author

@visitorckw visitorckw Sep 9, 2023

Choose a reason for hiding this comment

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

I believe the comment on line 57 of the code,/* TODO: clarify newlib internals */, is referring to this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the comment on line 57 of the code,/* TODO: clarify newlib internals */, is referring to this issue.

Exactly, you can describe more this time.

@visitorckw visitorckw force-pushed the refactor/time-retrieval-functions branch from 7c57a6c to 7b035e9 Compare September 9, 2023 04:01
This commit refactors time retrieval in utils.c by consolidating
common logic from rv_gettimeofday and rv_clock_gettime into a new
get_time_info helper function. This enhances code clarity, reduces
duplication, and improves maintainability.
@visitorckw visitorckw force-pushed the refactor/time-retrieval-functions branch from 7b035e9 to 8cee06c Compare September 9, 2023 17:30
@jserv jserv merged commit 9fffda5 into sysprog21:master Sep 9, 2023
@visitorckw visitorckw deleted the refactor/time-retrieval-functions branch September 10, 2023 00:58
vestata pushed a commit to vestata/rv32emu that referenced this pull request Jan 24, 2025
…val-functions

Refactor time retrieval functions
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants