Warning passing char ** to const char **

[markand]

Hi,

There is a warning because rx_main takes a const char ** (which is incompatible with char **) that can be seen with -Wall -Wextra.

Const correctness of double pointers is somewhat often misunderstood, I admit.

test/base64.c:389:32: warning: passing argument 4 of 'rx_main' from incompatible pointer type [-Wincompatible-pointer-types]
  389 |  return rx_main(0, NULL, argc, argv) == RX_SUCCESS ? 0 : 1;
      |                                ^~~~
      |                                |
      |                                char **
In file included from test/base64.c:21:
extern/librexo/rexo.h:6689:22: note: expected 'const char **' but argument is of type 'char **'
 6689 |         const char **argv)

[markand]

After some discuss with some folks at #C on libera chat I was unable to distinguish if your declaration of int main(int, const char **) seems valid. The paragraph 5.1.2.2.1 on C11 draft mention that only int main(void) and int main(int, char **) or other implementation defined are allowed. Other than that, I've tried compiling int main(int, const char **) with clang, gcc and msvc with all warnings enabled and strict pedantic mode and no one complained.

So that's not a real issue but I'd hear your feedback because const char ** is usually misunderstood anyway.

[christophercrouzet]

Salut ! :smile:

Thanks for raising this issue! A few points worth mentioning, please correct me if I'm wrong:

• rx_main is part of Rexo's API and, although its name is very similar to main, it is not the same as the C's program startup function so it does not have to abide by its rules (which is not what you were trying to say but I thought that it was worth mentioning nonetheless).
• it's a good practice to enforce const-correctness wherever possible to document the intention to only read the given pointed data and to help the developers to enforce this contract—here for example, Rexo does not mutate in any way the argv argument, so marking it with const seems to be the right thing to do, and this could be relaxed later on without breaking the API if there was a need for it.
• there's actually a thorough explanation of this specific issue on StackOverflow here and the TL;DR is to just ignore the warning... :cry:

As much as I hate compiler warnings myself (I try to hunt them down when possible), I'm not sure if it makes sense to comply with this one here since C's spec seems a bit off the mark for that one. Passing a char ** to a const char** parameter should be standard practice—the additional const restrictions of inner pointer parameters shouldn't affect the design of the code calling these functions. Once again, adding const should only serve as a documentation in the eyes of the caller.

In fact, if you are curious, there is a similar problem with free but this time the other way around, which is a bit more problematic. See Linus' answer here: https://yarchive.net/comp/const.html

All in all, I'd rather keep it this way, if that makes sense? Would you tend to agree?

[markand]

The thing is that const char ** means that it's only the pointer character that is const, you can still modify the pointer pointed-to in a function taking this as argument.

For example:

void take(const char **args)
{
    (*args)++
}

int main(int argc, char **argv)
{
    // argv[0] == "./a.out"
    take((const char **)argv);
    // argv[0] == "/a.out"
}

In fact, if we really would like to get a multidimensional pointer fully const'ed we would use const char * const *args but I admit it's ugly as well. With that declaration of pointer to const pointer to const char makes more sense but is also itself not assignable from a char ** value. But at least, it's not modifiable in any way.

const char * const *tab = NULL;

(*tab)++; // error
**tab = 'x'; // error
*tab = "new address"; // error

In the official C++ ISO guidelines, they also recommend to use const T * const *. Since the library didn't reach 1.0.0, maybe we could consider that notation instead 😀. Some libraries use that. The only thing I could not understand is that a C compiler throws a warning while a C++ compiler doesn't 🙄

[christophercrouzet]

I don't think that const char * const * adds any meaningful value over const char**.

The latter is basically synonym to passing a pointer by value that points to a const char array. The data that the caller owns, that is the char array, is guaranteed to not be able to change (well, at least that's the contract that is defined by the API). And that's all the caller cares about.

However yes, like you mentioned, we are able to change the value of the pointer itself if our function's implementation requires it, and there is nothing wrong with that since this doesn't make any difference for the caller. Only the pointer value that is local to our function's scope changes, not the pointed data! It's a feature, not something to guard against! :smile:

Let's substitute char * with T as you've already done it:

void foo(T x)
{
    // The argument x is passed by value and it's now tied to the scope of
    // this function—we can do that we want with x without fearing to mutate
    // the original data from the caller.
    x += 123;
}

void bar(const T *x)
{
    // This time x is passed as a const pointer. We cannot mutate the data
    // pointed to, which is what `const` guarantees, however the pointer value
    // is tied to the scope of this function so, here again, we should be able
    // to do what we want with that pointer, such as using it like an iterator
    // for example.
    while (*x != 0)
    {
        ++x;
    }
}

Just to make sure, I tried changing the signature of rx_main to take const char * const * but I still had the same warning:

passing 'char **' to parameter of type 'const char *const *' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]

The only thing I could not understand is that a C compiler throws a warning while a C++ compiler doesn't roll_eyes

The C and C++ specs simply disagree on many points, and that's one of them. Is there a good reason for this constraint in C? Maybe not, which would explain why it's only a warning, not an error! :smile:

[markand]

However yes, like you mentioned, we are able to change the value of the pointer itself if our function's implementation requires it, and there is nothing wrong with that since this doesn't make any difference for the caller. Only the pointer value that is local to our function's scope changes, not the pointed data! It's a feature, not something to guard against! 😄

No, it does modify to the caller site. That's why it's discouraged.

See https://wandbox.org/permlink/j1FjDiEvCqVqQvRd

[christophercrouzet]

Thanks for pointing this out @markand, much appreciated!

You're obviously right and I must admit that I never thought of dereferencing such pointers—which you alreadly correctly pointed out in your first example but I overlooked it (to my defence, it was starting to get late here :stuck_out_tongue:).

Oh jeez, that feeling of having been wrong for all these years!!!! :cry:

Now I'm going to have to go on a rampage to...

[markand]

Haha you're welcome.

I think everyone learn a new thing here and there everytime. I myself learnt recently that C99 designated initializers do not set padding bits to 0 in structs which lead into various painful bugs when using raw memcmp 😜.

[christophercrouzet]

Thanks again for your time and patience @markand!

I just pushed the commit 23c966f047dbef810078ae19ace4795dc1fd5649 with the changes to address the const issue, however we're still left with the compiler warning which... I think is fine to just ignore since I would personally favour const-correctness over an arguable warning?

[markand]

Yes we must ignore it. Don't know why C adds it and C++ doesn't as it's the correct way... Thanks for your understanding and working on this awesome C unit framework. I have few ideas of improvements that I'll try to add at some point :)

[christophercrouzet]

Thanks to your for your help and your kind words! :blush:

I have few ideas of improvements that I'll try to add at some point :)

I'm definitely looking forward to them! However I would like as much as possible to save you from spending time working on merge requests only for them to not being merged due to some possible disagreements, so I'd highly encourage you to first open new issues so that we can discuss things beforehand if that makes sense? :blush: