[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [sysvinit-devel] [patch] Get startpar building with clang
From: |
Mike Frysinger |
Subject: |
Re: [sysvinit-devel] [patch] Get startpar building with clang |
Date: |
Thu, 17 Apr 2014 15:50:13 -0400 |
User-agent: |
KMail/4.12.4 (Linux/3.14.0; KDE/4.12.4; x86_64; ; ) |
On Thu 10 Apr 2014 09:42:13 Petter Reinholdtsen wrote:
> I've submitted startpar for review with Coverty, and it reported four
> issues on <URL: https://scan.coverity.com/projects/1719 >. But the
> project is not yet accepted, so I can not figure out which issues this
> is. So I had a look at building it with clang, and it found a few
> issues with ignored return values and using a GCC extention. The
> following patch get rid of all warnings from Clang, but I am unsure if
> this is the correct fix. Any comments?
>
> The timerdiff() change make sense to me, but simply continuing to ignore
> exit values do not seem like a safe way forward. But I did not take the
> time to figure out why these exit values were ignored in the first
> place. Werner, do you remember?
>
> Index: startpar.c
> ===================================================================
> --- startpar.c (revision 178)
> +++ startpar.c (working copy)
> @@ -78,7 +78,11 @@
> # define attribute(attr) __attribute__(attr)
> #endif
>
> -#define timerdiff(n,l) (extension({
> (((n).tv_sec-(l).tv_sec)*1000)+(((n).tv_usec-(l).tv_usec)/1000); }))
> +long
> +timerdiff(const struct timeval n,const struct timeval l)
> +{
> + return (((n).tv_sec-(l).tv_sec)*1000)+(((n).tv_usec-(l).tv_usec)/1000);
> +}
way more paren in there than needed. they made sense in the old code as it
was a define, but this isn't.
would be nice to also fix the (lack of) spacing while you're here.
long
timerdiff(const struct timeval n, const struct timeval l)
{
return ((n.tv_sec - l.tv_sec) * 1000) + ((n.tv_usec - l.tv_usec) /
1000);
}
> - TEMP_FAILURE_RETRY(waitpid(splashpid, &status, 0));
> + (void)TEMP_FAILURE_RETRY(waitpid(splashpid, &status, 0));
i don't think that defeats the must_check_return attribute. you'd need
something like:
if (...) {
/* Ignoring return. */
}
-mike
signature.asc
Description: This is a digitally signed message part.