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

Conversion warnings #15

Open
EricOpitz-434 opened this issue Oct 16, 2024 · 4 comments
Open

Conversion warnings #15

EricOpitz-434 opened this issue Oct 16, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@EricOpitz-434
Copy link

Thanks for this beautiful library! I have integrated the library into my project and I have two comments:

  1. When I activate the gcc conversion warnings (-Wconversion) I get a bunch of warnings from cc.h.
  2. I'm using a map with a key of type const char *. I had to manually add this key type using CC_CMPR and CC_HASH. As I think this is a pretty common case, maybe you could consider adding this to the types that are supported out of the box.
@JacksonAllan
Copy link
Owner

Hi EricOpitz! I'm glad you're getting some use out of the library.

Regarding your points:

  1. The warning levels that CC was specifically designed to support are -Wall and -Wpedantic. I considered -Wconversion during early development, but GCC was complaining a lot about the passing of signed integers into the default hash and comparison functions for corresponding unsigned integer types, and I felt that defining a separate set of default (pass-through) functions for signed integers just to silence this optional warning was unnecessary. However, this is a rather minor change that I'm willing to make. I see now that -Wconversion also causes a lot of warnings about the conversion to double when a hash table's bucket count is multiplied by its maximum allowable load factor. All those warnings can probably be silenced by adding explicit casts. So in summary, I'll try to support -Wconversion in the next update, unless there are unforeseen complications.

  2. CC doesn't support const-qualified keys or elements in the general case because some containers—namely vectors, maps, and sets—move this data around internally. Obviously, this concern doesn't apply when we're talking about const-qualified pointers like const char *. Currently, string keys and elements are only supported by default via char *, so the user needs to manually cast away the const, where necessary, when inserting strings, which is hardly ideal for a library that advertises itself as "ergonomic" and is also bad from the standpoint of type safety. (Interestingly, GCC doesn't complain when the inserted key or element is a string literal.) It seems like addressing this issue will only require adding default hash and comparison functions for const char *, so again, I will do this in the next update. This issue is, by the way, the same issue flagged in Verstable, which is the basis of CC's hash-table implementation.

@JacksonAllan JacksonAllan added the enhancement New feature or request label Dec 6, 2024
@JayLCypher
Copy link

(Interestingly, GCC doesn't complain when the inserted key or element is a string literal.)

Just a comment on this:
(You can consider this one of the C standard's flaws)
String literals in C are typed as char *, but memory mutation of string literals is UB (because it's usually stored in non-write memory region), making literals effectively "immutable char *".
Example using _Generic shows this.

I see for your potential dynamic string issue #12 that this information may be useful, especially in combination with something like testing for literals using implicit literal concatenation and sizeof (literal)-1 for string length of literals, rather than calling strlen.

@fonghou
Copy link

fonghou commented Mar 5, 2025

It seems being fixed by commit b67fabe in dev branch

@JacksonAllan
Copy link
Owner

JacksonAllan commented Mar 6, 2025

The dynamic string implementation is mostly finished now. I still need to review/tidy up the code, write some more tests, and refine the documentation before releasing the new version.

As @fonghou pointed out, the new version will address both issues that @EricOpitz raised here, namely the absence of out-of-the-box support for const char * strings as map/omap keys and set/oset elements and conversion warnings with -Wconversion. Regarding the latter, the documentation of the new version will also explicitly state which compiler flags it supports (namely -Wall, -pedantic, -Wextra, and -Wconversion).

As for the dynamic string implementation, it mostly follows the API outlined in #12, but there are a few rough edges. For example, although the dynamic strings have default hash, comparison, and destructor functions for easy integration with other containers, there is currently no heterogeneous lookup (or heterogeneous insertion, which would also be nice). So if your map's key are of the type str( char ), there's not yet any way to perform a lookup using a regular char * string. Instead, you have to temporarily create a str( char ) just to perform the lookup.

I see for your potential dynamic string issue #12 that this information may be useful, especially in combination with something like testing for literals using implicit literal concatenation and sizeof (literal)-1 for string length of literals, rather than calling strlen.

This optimization could perhaps be used, but currently isn't, when inserting string literal arguments into strings. But the complication is that _Generic doesn't distinguish between a C string and C string literal unless you take the address. I have a few ideas about how to make it work in CC, but they might not be worth the extra, convoluted code that it would take to implement them.

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

No branches or pull requests

4 participants