Verstable icon indicating copy to clipboard operation
Verstable copied to clipboard

Avoid discarding const.

Open rsmarples opened this issue 1 year ago • 3 comments
trafficstars

Fixes #6.

rsmarples avatar May 21 '24 15:05 rsmarples

Regarding

- table->metadata = (uint16_t *)( (unsigned char *)allocation + VT_CAT( NAME, _metadata_offset )( table ) );
+ table->metadata = (void *)( (unsigned char *)allocation + VT_CAT( NAME, _metadata_offset )( table ) );

and

-   new_table.metadata = (uint16_t *)( (unsigned char *)allocation + VT_CAT( NAME, _metadata_offset )( &new_table ) );
+   new_table.metadata = (void *)( (unsigned char *)allocation + VT_CAT( NAME, _metadata_offset )( &new_table ) );

was Clang-Tidy complaining about this cast for some reason? The uint16_t * cast is necessary for C++ compatibility (as C++ disallows implicit conversion from void * to another pointer type).

And regarding

- static const uint16_t vt_empty_placeholder_metadatum = VT_EMPTY;
+ static uint16_t vt_empty_placeholder_metadatum = VT_EMPTY;

is this because Clang-Tidy complains about the (deliberate) casting-away of const in e.g. the _init function (table->metadata = (uint16_t *)&vt_empty_placeholder_metadatum;)? I'd rather leave vt_empty_placeholder_metadatum as const in order to make it very clear that the value of this "placeholder" is never changed. So maybe // NOLINT would be more appropriate here?

JacksonAllan avatar May 25 '24 18:05 JacksonAllan

Regarding

- table->metadata = (uint16_t *)( (unsigned char *)allocation + VT_CAT( NAME, _metadata_offset )( table ) );
+ table->metadata = (void *)( (unsigned char *)allocation + VT_CAT( NAME, _metadata_offset )( table ) );

and

-   new_table.metadata = (uint16_t *)( (unsigned char *)allocation + VT_CAT( NAME, _metadata_offset )( &new_table ) );
+   new_table.metadata = (void *)( (unsigned char *)allocation + VT_CAT( NAME, _metadata_offset )( &new_table ) );

was Clang-Tidy complaining about this cast for some reason? The uint16_t * cast is necessary for C++ compatibility (as C++ disallows implicit conversion from void * to another pointer type).

No, this was clang itself compiling it. I think clang17, or whatever is in Alpine Linux 3.20.0. It's necessary because char has an alignment of 1 and uint16_t an alignment of 2 so if it's off by one, it's a segfault on some platforms. On platforms which are sensitive to alignment it matters and clang warns you about it regardless of platform. The correct way (afaik) of telling a char pointer is correctly aligned to it's uint16_t pointer is to do an intermediate cast to void pointer. On C at least, you don't need an explicit cast of void * to any other pointer, hence (uint16_t *)(void *) is unnecessary. C++, I don't know as I don't use it.

And regarding

- static const uint16_t vt_empty_placeholder_metadatum = VT_EMPTY;
+ static uint16_t vt_empty_placeholder_metadatum = VT_EMPTY;

is this because Clang-Tidy complains about the (deliberate) casting-away of const in e.g. the _init function (table->metadata = (uint16_t *)&vt_empty_placeholder_metadatum;)? I'd rather leave vt_empty_placeholder_metadatum as const in order to make it very clear that the value of this "placeholder" is never changed. So maybe // NOLINT would be more appropriate here?

Actually it was gcc complaining about it. const is just a clue for correctness. You can unconst a pointer and write to it regardless. Dangerous if the pointer is a static assignment, but that is effectively what you are doing here because metadata where you are assigning to is changeable.

So both compiler errors. I'm on holiday for a week and don't have access to my sources right now, but from memory these are my CFLAGS.

CFLAGS+=	-g -Wall -Wextra -Wundef
CFLAGS+=	-Wmissing-prototypes -Wmissing-declarations
CFLAGS+=	-Wmissing-format-attribute -Wnested-externs
CFLAGS+=	-Wcast-align -Wcast-qual -Wpointer-arith
CFLAGS+=	-Wreturn-type -Wswitch -Wshadow
CFLAGS+=	-Wcast-qual -Wwrite-strings
CFLAGS+=	-Wformat=2
CFLAGS+=	-Wpointer-sign -Wmissing-noreturn
CFLAGS+=       -Werror

I think I included -pedantic and -std=c23 as well, but can't be sure.

rsmarples avatar May 25 '24 18:05 rsmarples

const is just a clue for correctness. You can unconst a pointer and write to it regardless. Dangerous if the pointer is a static assignment, but that is effectively what you are doing here because metadata where you are assigning to is changeable.

If the const variable is global, I believe compilers will typically store it in the program’s read-only “text” section. This is very desirable in this scenario, wherein the table’s metadata member could point to actual dynamically allocated metadata or this placeholder. It means that the program should crash if some bug causes a hash-table to attempt to write the the placeholder.

from memory these are my CFLAGS

Oh, I see. The issue here is the use of -Wcast-qual (in conjunction with -Werror). I've opened a new issue concerning support for optional diagnostics beyond -Wall -Wextra -Wpedantic (the diagnostics I already used when developing the library).

JacksonAllan avatar May 27 '24 15:05 JacksonAllan