I made a point here https://antirez.com/news/124 that comments are needed at the same time for different reasons, and different comments have differente semantical properties that can be classified in classes you very easily find again and again, even in very different code bases.

▲ commandersaki 4 days ago | parent [-]

This is a great post and meshes with how I like to comment as well. I like to break the so called rules and get a bit dirty when it comes to writing code and comments. My opinion which you state, is to remove the effort from the reader in needing to figure things out a second, third, or n-th time.

Here is one I wrote just to talk about iterating a loop in reverse:

    /*
     * We iterate the v6 prefixes in reverse from longest prefix length
     * to shortest. This is because the ipv6 address is a sequence of bytes,
     * and we want to perturb the address iteratively to get the corresponding
     * network address without making a copy for each perturbation as that
     * would be expensive.
     *
     * For example take address: abcd:abcd:abcd:abcd:abcd:abcd:abcd:abcd.
     *
     * With masks /112, /64, /8 we want to create the following network addresses
     * to lookup as follows:
     *
     * Lookup abcd:abcd:abcd:abcd:abcd:abcd:abcd:0000 in /112 bucket
     * Lookup abcd:abcd:abcd:abcd:0000:0000:0000:0000 in /64 bucket
     * Lookup abcd:0000:0000:0000:0000:0000:0000:0000 in /8 bucket
     *
     * In any other order aside from most specific to least, we'd have
     * to create copies of the original address and apply the mask each
     * time to get each network address; whereas in this case we can take
     * the same address and clear lower bits to higher bits as we go from
     * most specific to least specific masks without incurring any copies.
     */
    for (auto it = m_v6_prefixes.crbegin(); it != m_v6_prefixes.crend(); ++it)

Or here is another for masking a v4 address, but also explaining why a uint64 is used (this is calculated in a hot loop [same as the previous comment example], so I felt it was imperative to explain what is going on as there is very little room otherwise to optimise):

    for (const auto & [ mask_len, bucket ] : m_v4_prefixes)
    {
        /*
         * Example:
         *
         *   netmask 255.255.128.0 (/17) or 0xffff8000:
         *
         *   0xffffffff ffffffff << (32-17)
         *   --------------------------------
         *   0xffffffff ffff8000 (shifted left 15)
         *
         *   Applying against address 192.168.85.146 or 0xc0a85592:
         *
         *      (converted to uint64_t due to implicit integer promotion)
         *
         *      0x00000000 c0a85592
         *       &
         *      0xffffffff ffff8000
         *      -------------------
         *      0x00000000 c0a80000
         *      -------------------
         *               0xc0a80000 (after conversion to uint32_t causing
         *                           lower 32-bit truncation)
         */
        std::uint64_t mask = (~0ULL) << (32 - mask_len);
        std::uint32_t network_addr = addr & mask;
    
        const auto itt = bucket.find(network_addr);

▲

Karliss 3 days ago | parent [-]

Except your giant comment doesn't actually explain why it used uint64. Only place mentioning uint64 is integer promotion which only happens because you used 64bit integer, thus no explanation of why.

Was it done because shifting by amount equal or greater to integer width is undefined behavior? That would still not require storing result in 64bit mask, just shifting (~0ULL) would be enough. That would be a lot more valuable to explain than how bitwise AND works.

The first one also seems slightly sketchy but without knowing rest of details it's hard to be sure. IPV6 address is 128bits, that's 2 registers worth integers. Calculating base address would take 2 bitwise instruction. Cost of copying them in most cases would be negligible compared to doing the lookup in whatever containers you are searching resulting address. If you are storing it as dynamically allocated byte arrays (which would make copying non trivial) and processing it in such a hot loop where it matters, then seems like you have much bigger problems.

For my taste it would be sufficient to say "Iterate in reverse order from most specific address to least specific. That way address can be a calculated in place by incrementally clearing lowest bits." Having 2 paragraphs of text which repeat the same idea in different words is more distracting than it helps.

	▲	commandersaki 3 days ago \| parent [-]
		Sorry, I didn't explain uint64 was used. I wrote this many years ago so my memory was foggy, but I went through a few iterations using uint32 only to use branches for the masks. This was the only branchless way I could come up with at the time after a few attempts. I think the example was more to demonstrate that the algorithm was correct and I wasn't going to unit test it at that scope. As for the 128-bit addresses, we used boost ip::address_v6 to_bytes() as it appears there was no masking option. For my taste it would be sufficient to say "Iterate in reverse order from most specific address to least specific. That way address can be a calculated in place by incrementally clearing lowest bits." Having 2 paragraphs of text which repeat the same idea in different words is more distracting than it helps. Ah apologies, too late now, should've mentioned it in the PR. But I expected it would ruffle some feathers, I don't care for conventions or other people's quibbles. As long as it improves understanding for the reader, regardless of how "redundant", then mission accomplished.