Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid discarding const. #7

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rsmarples
Copy link

Fixes #6.

The compiler cannot know the allocation is aligned to uint16_t
and this avoids a cast alignment warning emitted by clang.
@JacksonAllan
Copy link
Owner

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?

@rsmarples
Copy link
Author

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.

@JacksonAllan
Copy link
Owner

JacksonAllan commented May 27, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cast discards 'const' qualifier from pointer target type
2 participants