Enforce const Where Applicable

[PingXie]

it would be ideal if we express the "constness" of the returned sds string explicitly. Similar callouts to other helper functions returning plain pointers. That said, since we have never enforced "constness" in the past in our code base I suspect this could turn into quite a code churn. So definitely not a blocker for this PR (and it is primarily for code hygiene).

typedef const char* sds_const; // in src/sds.h

sds_const scriptingEngineGetName(scriptingEngine *engine) {

Originally posted by @PingXie in #1312 (comment)

[ranshid]

I like this proposal.

1. I think the name sds_const can be somewhat misleading? it might imply char * const and not const char *. I would suggest define it as const_sds.
2. note that const sds_const will be const char * const which is fine I guess and makes me wonder if we should not force the constness over the pointer as well?. eg:
   typedef const char* const sds_const;
   which would also help prevent cases of:

sds_const a = sdsnew("one two three");
sds_const b = sdsnew("four five six");
a = b;

which can result in memory leak

[zuiderkwast]

I like the name const_sds better than sds_const.

We need to add only what can't already be expressed. That is constness for the sds content. The pointer-constness can already be expressed. Coupling pointer-constness with content-constness is not necessary. It's not normal style in C.

The pointer-constness is just about not overwriting a local variable. It's up to each function how it uses local variables IMO. It's possible can use const for int variables or any pointer types. It's a matter of style.

[ranshid]

It's not normal style in C.

I think that the decision to treat sds as a concrete type makes it more logical to enforce the pointer assignment as well. In all cases I can think of we do not want to change the "pointer" sds as well since than we lose the sds content as well.
I just think that stating const const_sds is strange :)

[zuiderkwast]

@PingXie What do you think about @ranshid's suggestion to make it const + const, i.e. const char * const?

[zuiderkwast]

2. typedef const char* const sds_const;
which would also help prevent cases of:

sds_const a = sdsnew("one two three");
sds_const b = sdsnew("four five six");
a = b;

which can result in memory leak

@ranshid Constness doesn't prevent a memory leak. Just let the variable run out of its scope and it leaks.

{
    sds_const a = sdsnew("one");
}

What it does though is that it prevents reassignement of a variable, so code like this becomes illegal:

sds_const *array = something();
sds_const s;
int i;
for (i = 0; i < n; i++) {
    if (is_our_wanted_string(array[i])) {
        s = array[i]; // error: assignment of read-only variable ‘s’
        break;
    }
}
printf("%s", s);

We don't have this limitation for any other type. If we want to prevent reassignment of for example an int, we use const int, but we don't use it when we need to update it, such as computing a sum...

const int * const array = something();
const int sum = 0;
for (i = 0; i < n; i++) {
    sum = sum + array[i]; // error: assignment of read-only variable ‘sum’
                          // But just use `int sum`! It has nothing to do
                          // with the constness of `array`. It's per variable,
                          // not per the content.
}

I hope these examples show the problem of tying pointer-constness (single-assignment) and content-constness (mutability) together.

[PingXie]

@PingXie What do you think about @ranshid's suggestion to make it const + const, i.e. const char * const?

I agree with you. I don't think we should apply constness to the pointer, which is a separate concern and usually not a big issue. The main point is to enable static checks for unintended data modifications and compile time optimizations. It is also not about preventing memory leaks.

I like const_sds too.

[ranshid]

I think that there is not much concern dropping the constness on the pointer level (as we just did)
I just feel it is makes the sds act more like a standalone type, so "assignment operator" like actions would not be possible (which is fine IMO).

when you define something as const, you want to ensure the content is not changed in anyway (even by reassigning the value) which is currently not guaranteed