On Donnerstag, 25. August 2016 14:39:56 CEST Jonathan Haws wrote:
Signed-off-by: Jonathan Haws jhaws@sdl.usu.edu
Added time_subtract() utility function to use for simply subtracting one timespec from another.
Please add a small changelog when you send a new revision (after the first "---"). Please also add the v2/v3/v4/... version to your [PATCH...] prefix for patches replacing an older version. These are generated automatically by git- format-patch when you add the parameter "-v 2" or "-v 3" or ....
A good example is https://patchwork.open-mesh.org/patch/16663/
Please use the format described in https://www.kernel.org/doc/Documentation/SubmittingPatches
The canonical patch subject line is:
Subject: [PATCH 001/123] subsystem: summary phrase
The canonical patch message body contains the following:
- A "from" line specifying the patch author (only needed if the person sending the patch is not the author).
- An empty line.
- The body of the explanation, line wrapped at 75 columns, which will be copied to the permanent changelog to describe this patch.
- The "Signed-off-by:" lines, described above, which will also go in the changelog.
- A marker line containing simply "---".
- Any additional comments not suitable for the changelog.
- The actual patch (diff output).
You don't seem to follow these rules yet. Instead you seem switch the Signed- off-by: line with the body of the explanation. So a git-am with -s (as the maintainers do here) would then produce this mess:
alfred: Adding time_subtract utility function
Signed-off-by: Jonathan Haws jhaws@sdl.usu.edu
Added time_subtract() utility function to use for simply subtracting one timespec from another. Signed-off-by: Sven Eckelmann sven@narfation.org
[...]
index c5df945..884a1a7 100644 --- a/server.c +++ b/server.c @@ -372,7 +372,10 @@ int alfred_server(struct globals *globals)
while (1) { clock_gettime(CLOCK_MONOTONIC, &now);
time_diff(&now, &globals->sync_period, &now);
/* subtract the synchronization period from the current time */time_subtract(&now, &globals->sync_period, &now);
I'm guessing Simon meant to have the comment in the first patch. I don't get why you now introduce a time_subtract which is just a copy of time_diff (without the return value).
+void time_subtract(struct timespec *tv1, struct timespec *tv2,
struct timespec *tvout) {- tvout->tv_sec = tv1->tv_sec - tv2->tv_sec;
- if (tv1->tv_nsec < tv2->tv_nsec) {
tvout->tv_nsec = 1000000000 + tv1->tv_nsec - tv2->tv_nsec;tvout->tv_sec -= 1;- } else {
tvout->tv_nsec = tv1->tv_nsec - tv2->tv_nsec;- }
Have you checked the glibc documentation about this topic (search for timeval_subtract)?
https://www.gnu.org/software/libc/manual/html_node/Elapsed-Time.html
This might be relevant when you want to create an function which does handle more things than time_diff currently does. Otherwise I don't see a reason why we just have the same code twice in util.c
- return;
+}
Why a return here?
Btw. I am seeing whitespace warnings when applying the patches. Maybe you can check them:
Applying patch #16667 using 'git am' Description: [1/2] alfred: Externalized synchronization interval Applying: alfred: Externalized synchronization interval .git/rebase-apply/patch:89: trailing whitespace. ALFRED_INTERVAL setting of 10 seconds will be used. Fractional seconds are warning: 1 line adds whitespace errors. Applying patch #16668 using 'git am' Description: [2/2] alfred: Adding time_subtract utility function Applying: alfred: Adding time_subtract utility function
Kind regards, Sven