Re: [PATCH] maint: avoid clang -Wint-to-pointer-cast warning

[Bob Proulx]

Pádraig Brady wrote:
> Subject: Re: [PATCH] maint: avoid clang -Wint-to-pointer-cast warning

What was the text of the original warning?  I think that would be
interesting to review.  I think it would provide an additional clue.

> * src/chroot.c: Explicitly cast int to pointer type.

> -          g = (struct group *)! NULL;
> +          g = (struct group *) (intptr_t) ! NULL;

I would like to look at this cast to a cast again.  It already had a
cast before.  I think adding an additional cast is one too many.  I am
surprised that it isn't still tripping over a warning.  And I think
later that the multiple cast might itself be the source of yet a
future warning.

And it is inverting zero?  I am not sure what it is doing.  I have not
looked at this code before so let me ramble the details as I learn
them.  Pseudo-code:

  char const *groups
  char *buffer = xstrdup (groups);
  tmp = strtok (buffer, ",")
     struct group *g;
     ...handle forced numeric +42 cases...
  ... g is either a "struct group *" pointer or NULL ...
    g = getgrnam (tmp);
  ... if a numeric uid supplied then use g a flag variable.  Don't
  allow it to be set to NULL which would indicate a getgrnam()
  failure.  Indicate sucess by it not being NULL.
    g = (struct group *)! NULL;
  if (g == NULL)
    ... handle the error ...

I think that is the logic of what it is doing.  Frankly I ran through
this very quickly.  I could be wrong.  It was only the "! NULL" and
multiple casts of it that triggered my immune response forcing me to
jump in.

Note that in C the NULL macro is effectively 0.  In C++ it is
different but sticking to C only for now could say that is 0.  So
therefore the "g = ! 0" which is basically 0xFFFFFFFF the all ones
value.  And so we see the problem of the pointer casting to make that
happy.  But it does not feel good to me.

  g = (struct group *) (intptr_t) ! NULL;

Adding another cast on top of the existing feels worse to me.  And is
it really needed?  I think not.

At the least I would create a dummy.  Then assign the dummy.  That
would be robust in spite of anything.  It would be logically correct.

  struct group dummy_but_true = { 0, 0, 0, 0 };

  g = dummy_but_true;
  if (g == NULL)
    ... handle the error ...

I don't like needing to enumerate out all of the fields.  But at least
then there is no type issues dealing with the g pointer later.

I think instead of either I would rather see a plain boolean that
explicitly says what it is doing there.  "the_parameter_is_numeric" or
some such.  (I didn't have a good name which is why I created a much
too long named one.  Obviously it needs a something simpler there.)  I
think it is not so good to overload this pointer in the way that it is
being overloaded.

And sorry but that is all of the time I have at this moment.  I hope
to look at it more again later.  And chown too to see how it handles it.

Bob