[PARSE] Constness of the API

[frozar]

Update the prototype of every parse functions to make them constness: where the const keyword should be, just put it.

When I write code, in the client side, I try to use const wherever it's possible. If in the library side, the const interface is not present, I have to remove the const keyword from the client side code (or use a temporary variable, or another trick) to be able to use the PCL functions. With the const API, from the client side you can use indifferently mutable or const variables.

Even if this PR changes the API, I don't think that it really breaks it. From the client side, every application should remain the same.

Also, I'm not able to say if this PR breaks ABI or not...

[taketwo]

const is good. But I don't get why everything became a reference all of a sudden?

[frozar]

@taketwo there is no real reason to use reference I must say. If I have to change a function to get it constness, I would change this:

func (int i);

To this:

func (const int & i);

I don't know the impact of using a reference in these cases. Do think that using reference maybe harmful in some situation?

[taketwo]

Remember, a reference is a pointer in the end. The only difference is that it can not point nowhere and that you do not need to dereference. But it is a pointer! So when you pass a reference, you are actually passing a memory address. For large objects this is a good thing, because you avoid copying. But for small, primitive types it is an overhead! int takes less space than a pointer to int. So there is absolutely no reason write func(const int& i) instead of func(int i).

So the only change that I would support is switching from char** argv to const char** argv.

[frozar]

I agree, I was thinking about 64 bits pointer compare to sizeof(int) which is usually 4 bytes, so 32 bits.

So let say instead of func (const int & argc) I will use func (int argc), but for argv you cannot just use const char ** argv. Let look at the following snippet:

int main(int argc, char ** argv) {
  const char *const * argv_cp_0 = argv;
  const char * * argv_cp_1 = argv;
}

When you try to compile it you get the error:

const_convertion.cpp: In function ‘int main(int, char**)’:
const_convertion.cpp:5:30: error: invalid conversion from ‘char**’ to ‘const char**’ [-fpermissive]
  const char * * argv_cp_1 = argv;

So for argv you need at least const char *const * argv as prototype. So why not use const char *const *const argv?

[taketwo]

I bet out of 100 C++ programmers only 1 could figure out without Google what exactly

const char *const * const argv

implies. Can you explain what this type is? ;)

[frozar]

I adgree that it is really not common and everybody in messing around what is const when you see char *const * for example.

To explain what exactly is:

const char *const *const argv

I recommand to read this article which goes in great details about types in C/C++.
According to this article, the above declaration means: const pointer to const pointer to const char.

Ok, maybe you're not glad with that, so I create a tiny example which give the difference between const char *const * and const char *const *const:
https://github.com/frozar/model/blob/6799559ed034a54297653a84e6ef0fd7ff82f572/cpp/const_attempt.cpp

In short, if you have const char *const *const var, you are not allow to change the address in var after its initialisation. To be complet, image you have a const char * *const var1, this means that you cannot change the address in var1 but you can change the address at var1[0] for example.

I hope clearer now.

For this PR, I advocate to use const char *const *const argv as prototype. Opinions?

[SergioRAgostinho]

For this PR, I advocate to use const char *const *const argv as prototype. Opinions?

It's mildly cryptic but strict and precise, I'm game with it. Please preserve the spaces between the const qualifier and the pointer type i.e. const char* const * const argv.

[frozar]

Nice, I made a proposition in the last commit.

[SergioRAgostinho]

I see the spacing is fixed and no native types got converted to const references. If I'm not mistaken this doesn't really break the API: before you couldn't use const types, now you can so it shouldn't break functionality for the old cases, just add a new one.

It's ok to merge from my side. @taketwo since you were involved in the discussion I'll wait for your final comments/request/merge.

[taketwo]

My personal opinion is that this looks outright crazy:

find_switch (const int argc, const char * const * const argv, const char * const argument_name)

There are 6 consts for three variables. Three of them are not necessary, it can be:

find_switch (int argc, const char * const * argv, const char * argument_name)

This is easier to read, does not affect const-correctness, and enables Fabien's use cases.

[SergioRAgostinho]

I don't really have a strong opinion on this one so I'll yield to @taketwo 's sanity check. @frozar please modify it according to his comments.

[frozar]

@taketwo : you don't want to keep the const for argc?

[taketwo]

I'm an advocate of not using const for by-value arguments. You can not modify the variable at the call source through such argument anyway.

Sergio would argue that this const adds protection from shooting yourself in a leg and accidentally modifying the argument inside the function and using modified value later in the same function. I think such things happen extremely seldom and the price you pay by bloating the interface declaration with unnecessary const is higher.

Edit: in a way, the fact that your by-value argument is const is an implementation detail. The user of the function does not care if you modify the argument or not.

[frozar]

Ok, so in this case you forbid the use of const prototypes, for example toto(const int argc), from the client side in the case toto is calling a PCL parse function with argc as argument. That's not bad but it should be an aware choice 😉 (and should be aware of this choice too).

[taketwo]

I'm not sure I understand your idea.

There is no problem to can void parse(int arg) with a const variable as an argument.

[frozar]

@taketwo You're right, there is no problem in what is proposed in this PR.

I have a doubt about it because previously I had some issue but it is in the case I used references, like in the following:

void toto(int & i)
{
  std::cout << "just & i: " << i << std::endl;
}

void tata(const int & i)
{
  std::cout << "const & i: " << i << std::endl;
  toto (i);
}

int main(int argc, char ** argv) {
  int i = 42;
  tata (i);
  return 0;
}

In this case, you cannot pass a const int & i as a int & i (calling toto() from tata()) and this is because of the references, that's it.

I'm OK with this PR 😸

[taketwo]

That's because of the references, exactly.