Design review
If you’re okay with the risk of collision, then I don’t see any major problems with this design. It should work fine.
However, it strikes me as a little over-engineered. It’s the kind of solution where you’re being so clever, you may actually be outsmarting yourself.
The first thing that bugs me about it is that using hashing like this makes potential collisions extremely difficult to predict, or to spot when they happen. Suppose you know someone has used "foo"_e, and you want to use "bar"_e. Now… will those two strings generate the same hash? Probably not, sure, but… how can you know? Practically, the only way to know is to actually check the hashes. You can’t tell just by looking at the strings.
Where this becomes really problematic is when something starts acting strangely in your program. For example, let’s say your program misinterprets certain frames. Because you have allowed for the possibility of hash collisions, that means you have one more thing to add to the checklist of possible bug sources. And to really make that headache pound, to rule that possible bug out completely, you now have to check the hashes for every string used by every module in the program. And remember, you can’t simply tell at a glance whether two strings’ hashes collide… you have to check every one. Unless you have another way to rule out that possibility, this will be a MASSIVE pain in the ass.
The other thing that bugs me about this hash design has nothing to do with collisions, but rather the simple problem that whenever you want to get any information about what is really going on in your program—for debugging, or logging, or whatever—you are going to have a hell of a time getting useful information. For example, let’s say that you notice that an error occurs whenever handling a frame with type 1,340,073,580. Great! So… what exactly does that mean? What was that nice, pretty descriptive string in the source code that hashed to 1,340,073,580? 🤷🏼
Hashing seems silly for a task like this. What, really, do you gain by transforming the lovely little descriptive string irreversibly to a number? Sure, it means you can use any arbitrary string of any arbitrary length… but… do you really need that kind of flexibility? Does it really need to be possible to write all of “Hamlet” as the description for a frame type? Or the poop emoji?
Consider instead putting some restrictions on the string. For example, let’s say that it can only contain Basic Latin characters, and can be a maximum of 8 characters. With that, you can fit it into 64 bits, which means you can trivially translate it to a 64 bit number. Conceptually, that’s kinda like a hash… except it is bijective: there is a 1-to-1 mapping of string to hash, and the mapping is reversible. You get all the benefits of a hash—like lightning fast compares—with the only downside being a limitation on the strings. And you get the additional benefits of meaningful values and easy debugging.
8 characters not enough? Well, then, you could limit the characters to only upper- and lower-case English letters, and digits, which is a total of 62 unique characters, allowing you to add two more characters to get 64… let’s say space and NUL. So you can fit a–z, A–Z, 0–9, space, and NUL in 6 bits. In a 64-bit number, that gives you 10 characters (plus 4 bits of wiggle room).
(And if you really want to use the poop emoji, you could say that the string must be UTF-8 encoded with a max length of 4 code units. That would make your value 32 bits, and your strings could be 4 letters (“Abcd”), or 2 Greek or Cyrillic letters (“ум”), or a single Chinese, Japanese, or Korean ideograph (“知”)… or… well… “💩”. Use 64 bits, and you can have two poops. 👍)
Allow me to illustrate with some quick and hacky code:
// First, create a bespoke type for your things.
//
// Let’s say, for example, we’re making a type for frame type tags.
class frame_type_tag
{
std::uint_fast64_t _value; // "64 bits should be enough for anybody."
//
// Srsly, tho, 64 bits means 8 UTF-8 code
// units, assuming char is UTF-8 encoding.
// (And you could make that required, with a
// little extra work.)
// Private constructor, so objects can *only* be created using UDLs.
constexpr frame_type_tag() noexcept = default;
public:
// So we can compare:
constexpr auto operator==(frame_type_tag const&) const noexcept -> bool = default;
// So we can nicely print:
template <typename Char, typename Traits>
friend auto operator<<(std::basic_ostream<Char, Traits>& o, frame_type_tag ft) -> std::basic_ostream<Char, Traits>&
{
auto buffer = std::array<char, 9>{};
for (auto i = 0; i != 8; ++i)
buffer[i] = ft._value >> ((64 - 8) - (i * 8));
return o << buffer.data();
}
friend consteval auto operator""_frame_type(char const*, std::size_t) -> frame_type_tag;
};
// Ignore the actual algorithm; I just slapped it together. It should
// illustrate the point, though.
consteval auto operator""_frame_type(char const* s, std::size_t n) -> frame_type_tag
{
// Here you can verify that the string is 8 or less characters, that it's
// all alphanumeric characters, or whatever else you want to check.
constexpr auto max_n = std::size_t(8);
auto ft = frame_type_tag{};
ft._value = 0;
auto i = std::size_t{};
for (; i != std::min(n, max_n); ++i)
ft._value = (ft._value << 8) | s[i];
for (; i != max_n; ++i)
ft._value <<= 8;
return ft;
}
auto main() -> int
{
constexpr auto ft1 = "bla1"_frame_type;
constexpr auto ft2 = "bla2"_frame_type;
std::cout.setf(std::ios_base::boolalpha);
std::cout << "frame type 1 is " << ft1 << "\n";
std::cout << "frame type 2 is " << ft2 << "\n";
std::cout << "ft1 == ft1: " << (ft1 == ft1) << "\n";
std::cout << "ft1 == ft2: " << (ft1 == ft2) << "\n";
}
I am glossing over one really important complication, and that is that if the character encoding is different on two machines, the hash will be different. That is true both for my “pseudo-hashes”, and your actual hashes. If you want consistent values for strings across platforms, you will need to make sure the character encoding is consistent. That’s not hard to do… but it will take some doing.
If you don’t care about having consistent hashes across platforms, then you don’t need to worry about it.
The bottom line is this: Hashing makes sense for arbitrary strings… but do you really need arbitrary strings. Limit the length and/or the allowed characters, and you can encode the strings directly by transforming them into a number, which is a bijective function, with all the benefits that brings.
On the other hand, if you really do want to allow completely arbitrary strings, then hashing is about all you can do. In that case, you’ll have to live with the problems.
Code review
constexpr uint32_t operator""_e(const char *const str, const size_t length) {
First, it’s std::uint32_t. And you need to include <cstdint>.
However… what most people don’t realize is that std::uint32_t is not portable. See for yourself: note how it says “optional” next to the std::uintXX_t types?
For portability, what you probably want to use is std::uint_fast32_t. But that creates a new problem, because std::uint_fast32_t is not necessarily going to be exactly 32 bits. If you want only 32 of hash value, then you need to clip it to 32 bits. The easiest way is probably to change the return line to return static_cast<std::uint_fast32_t>(result & 0xffff'ffffu);
It should also be std::size_t. And for that, you need to include something that defines std::size_t. You’ve got options, but the most lightweight choice is probably <cstddef>.
Now, as a matter of style, const char *const is weird. I’m not referring to the weirdness you get because of west-const style (though, I’m not a fan). I’m referring to the fact that in C++, we prefer to put the type modifier with the type.
In other words:
const char *const is C styleconst char* const is C++ stylechar const* const is also C++ style, east-const 4 lyfe
A more important matter of style is that you should be using a custom type, not std::uint32_t (or std::uint_fast32_t). Using a naked integer type like std::uint32_t means your converted hashes can be mixed up with… well, pretty much anything. You will be able to write absolute gibberish like "bla1"_e + "bla2"_e or std::sqrt("bla3"_e).
What you should do is create a custom type for these things. It can be a simple wrapper around std::uint32_t, but with a more restricted interface. You won’t need the arithmetic stuff, for example. Pretty much all you should need, really, is comparisons. In the example I made in the design review, it just has comparisons, and a print function. You probably don’t need much more than that.
Finally, you don’t say which version of C++ you’re targeting, but if it’s C++20 or better, you might want to use consteval rather than constexpr. The only difference is that constexpr means “could run either at compile time or at run time”… while consteval means “will run only at compile time”, which makes more sense for this.
(Also, if you really do want to allow any arbitrary string, then there’s no way it can fail, so you could as well declare this noexcept.)
Now, you have a number of very subtle bugs in your code because of using the wrong types, and the wrong literals. Let’s start at the top:
uint32_t result { 0x811c9dc5 };
I already mentioned that uint32_t should be std::uint32_t, and in any case it isn’t portable. But putting all that aside… 0x811c9dc5 is a (probably) signed int (or larger) literal. This is problematic for two reasons:
- It’s (probably) signed. You want an unsigned literal.
- It’s (probably) an
int, which may be larger than 32 bits, making this a narrowing conversion… which is a problem, because you’re using braces, which prevent narrowing conversions (which is a good thing).
To make sure this works portably, you should enforce the type you want by using parentheses, not braces, or by using static_cast:
auto result = std::uint32_t(0x811c9dc5u); // note: parentheses, not braces
// or:
auto result = static_cast<std::uint32_t>(0x811c9dc5u);
But of course, you should use a portable type like std::uint_fast32_t instead of std::uint32_t.
Next up is the loop. Naked for loops are a code smell, mostly because they are shockingly hard to get right and… surprise, surprise… yours isn’t right.
Are you compiling with warnings turned on? If you were, you should have gotten warnings about mixed signed and unsigned comparisons, and maybe also narrowing. You should always compile with all warnings turned on.
int is signed. std::size_t is unsigned. Don’t mix signed and unsigned types. That way lies madness.
More dangerously, though… you don’t know that int is the same size as (or larger than) std::size_t. If someone passes a large enough string in, you could increment right past the max value of int… and for a signed type, that means UB.
What you should be using here is not int, but std::size_t:
for (auto i = std::size_t{}; i != length; ++i)
Note that I also changed the < to !=. This is important for the case where length happens to be the max value of std::size_t. This is the correct way to write this loop.
But all these subtle complexities are why the smart thing to do is not write a loop at all, and instead use an algorithm. For example:
consteval auto operator""_e(char const* s, std::size_t n) noexcept -> std::uint_fast32_t
{
auto const init = std::uint_fast32_t(0x811c9dc5u);
auto const hasher = [](std::uint_fast32_t hash, char c) { return std::uint_fast32_t((hash ^ c) * 16777619u); };
return std::accumulate(s, s + n, init, hasher) & 0xffff'ffffu;
}
That’s not only safer than the naked loop, it’s potentially faster, for a number of reasons. (For example, in your loop, you ask that a pointer calculation and dereference be done for every byte with str[i], whereas the algorithm could load a bunch of bytes at a time, and feed them to the hasher directly. In reality, modern compilers are so smart they will probably optimize the differences away. However, the point stands: the algorithm is simpler, safer, and, at least theoretically, more efficient.)
