[systemd-devel] [PATCH] shared: fix cppcheck warnings

[Lennart Poettering]

On Tue, 14.10.14 21:50, Boris Egorov (egorov at linux.com) wrote:

> Minor patch. Fixes defects found by cppcheck: do not check if unsigned
> value is const, reduce scope of a few variables.
> 
> Signed-off-by: Boris Egorov <egorov at linux.com>

Hmm, semi-enthusiastic about this patch, see below.

> ---
>  src/shared/macro.h | 2 +-
>  src/shared/ring.c  | 7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/shared/macro.h b/src/shared/macro.h
> index 9ee332c..372475b 100644
> --- a/src/shared/macro.h
> +++ b/src/shared/macro.h
> @@ -295,7 +295,7 @@ static inline size_t IOVEC_INCREMENT(struct iovec
> *i, unsigned n, size_t k) {
>          for (j = 0; j < n; j++) {
>                  size_t sub;
>  -                if (_unlikely_(k <= 0))
> +                if (_unlikely_(k == 0))

I think cppcheck is really confused here... I really prefer the <=
checks in this case, even if the type would never allow negative
values, and thus == checks would suffice. Why? simply because this
code makes the logic independent of the precise type, it will always
work correctly. 

> +++ b/src/shared/ring.c
> @@ -83,8 +83,6 @@ size_t ring_peek(Ring *r, struct iovec *vec) {
>   * return the number of bytes copied.
>   */
>  size_t ring_copy(Ring *r, void *buf, size_t size) {
> -        size_t l;
> -
>          assert(r);
>          assert(buf);
>  @@ -92,6 +90,8 @@ size_t ring_copy(Ring *r, void *buf, size_t size) {
>                  size = r->used;
>           if (size > 0) {
> +                size_t l;
> +
>                  l = r->size - r->start;
>                  if (size <= l) {
>                          memcpy(buf, &r->buf[r->start], size);
> @@ -110,7 +110,6 @@ size_t ring_copy(Ring *r, void *buf, size_t size) {
>   */
>  static int ring_resize(Ring *r, size_t nsize) {
>          uint8_t *buf;
> -        size_t l;
>           assert(r);
>          assert(nsize > 0);
> @@ -120,6 +119,8 @@ static int ring_resize(Ring *r, size_t nsize) {
>                  return -ENOMEM;
>           if (r->used > 0) {
> +                size_t l;
> +
>                  l = r->size - r->start;
>                  if (r->used <= l) {
>                          memcpy(buf, &r->buf[r->start], r->used);

i you rebase and split out the ring.c changes I'll commit them, though
they don't really matter much....

Thanks,

Lennart

-- 
Lennart Poettering, Red Hat