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