5 ms·

Just looking at the API "sds" seems to just be a typedef for char* - unfortunately, that means that accidentally passing a char* as an sds into any of the funct

• 0xdead 7y ago
  You can "accidentally" do a lot of things that any compiler in the world won't warn you about. This isn't an excuse to not use a good library/tool.
• liuliu 7y ago
  It is not a real issue in modern days. You just enable address sanitizer in all your debug builds and these issues are obvious at runtime (still not perfect, but an improvement from dark days).
  • saagarjha 7y ago
    Address sanitizer can only catch errors on branches your debug build exercises.
    • liuliu 7y ago
      For redis, I think given the code coverage on test cases, that shouldn't be a concern. TBH, I think any C code that doesn't have a coverage is not a functioning code. Too much bugs can only be caught at runtime (thankful to the sanitizers-family / valgrind).
• chacham15 7y ago
  What you do is something like the following: typedef struct sds_s { char data[0]; } sds; Which makes them essentially equivalent, but not from a type perspective. Then to handle conversions, you add explicit functions a la char* sds_cstr(sds *str); Then you have full type checking to help you (and an additional advantage if your data format changes).
  • electrograv 7y ago
    The downside to using a non-inlined conversion function is that (1) it will be slower than just accessing the member directly, and (2) it’s much more verbose. Why not just use the member explicitly? This way, converting a STS string into a legacy C string could be as simple as writing “.cstr” (or if you just be as terse as possible, it could be defined as “.s” as another poster suggests). In this case, compromise of increased code verbosity is extremely minor at worst (just a few characters). At best, this extra explicitness can actually be seen as a good thing for code readability (not to mention the huge benefits we’re discussing of type safety). So the question then is: Why doesn’t SDS do this? The actual library uses a regular C typedef (which is unsafe for the reasons described above).
    • chacham15 7y ago
      I think you're missing the point here, the conversion function is trivially inlinable: char* sds_cstr(sds *str){ return &str[0]; //you could probably just cast too } The other posters solution is actually not equivalent in this way (his sds is convertable to char* instead of sds*).
      • electrograv 7y ago
        Even an inlined function only has value if our goal is to open the door to implementation changes in the future. But because the current implementation is zero-cost (an inlined no-op), any behavioral change will necessary compromise performance. Otherwise (if we want to retain the zero-cost guarantee of converting to a C string), directly accessing the member variable is: functionally equivalent, simpler, more concise, more readable, more explicit.
        • chrchang523 7y ago
          I actually prefer the inline function here (while making the member char* private when compiling as C++), since that mostly prevents assignment to the member variable in settings where that would not be a valid operation. With that said, I agree that the additional verbosity is a significant drawback, and I can understand going with the slightly-less-safe option for that reason alone.
• pantalaimon 7y ago
  > Considering this is C, there is no way to prevent this easily since you can't express a type that is one-way convertible (i.e. sds -> char* ok, char* -> sds not ok). typedef struct { char *s; } sds_t; will produce an error if you try to pass a char* instead. http://codepad.org/CmrskN9n http://codepad.org/CmrskN9n
  • deleted 7y ago
    [deleted]
• chrchang523 7y ago
  My preferred approach for handling this type of scenario is to write C code that is valid C++, while adding a bunch of C++-specific mostly-compile-time safety checks which can't be expressed in pure C. It's not perfect, but it has helped me reduce the impact of footguns without sacrificing the interoperability advantages of C. (And as other commenters have noted, wrapping the char* in a struct addresses the immediate footgun in question.)
• antirez 7y ago
  One of the main points of the library is the ability to pass SDSs where char* is expected without doing anything. So this is a "feature" in the author's spirit.
  • electrograv 7y ago
    As another reply has suggested, you can design the wrapper struct so you can simply write: legacy_fn(my_text.s); Versus the current: legacy_fn(my_text); I don’t think saving two characters per legacy function call is even remotely worth the loss of static type safety (which risks serious memory corruption and/or security holes, which are entirely preventable at compile-time in this way). In fact, I even find the explicitness more pleasantly and clearly readable: I like being able to know at-a-glance when types are changing, especially in a language as unsafe as C. Lastly, if you can tolerate just using some of C++‘s features, you can define a no-compromise solution: A type (still represented by a single pointer under-the-hood) that will implicitly convert (with zero runtime cost) into a C string but not vice versa.
    • jhallenworld 7y ago
      A main problem with C++ is that it uses .c_str() instead of something nice like .s for this.
• cryptonector 7y ago
  The right way to do this would be: typedef struct sds { char *s; } sds; Now you get type safety: you can't accidentally pass a plain 'ole C string to an sds function. But you do need to access the 's' field to get the C string for passing to non-sds functions.
  • shawxe 7y ago
    This is fine, but it somewhat defeats the purpose of the opaquely prefixed header. I think the idea is just to be able to treat an SDS like a normal string. Unfortunately, there's no way to have that property not be commutative in C.
    • cryptonector 7y ago
      Yeah, but almost perfect ergonomics is good enough, and type safety >> ergonomics.