15
\$\begingroup\$

I've recently picked up a book that gives various "modern" C++ challenges/solutions. One of the first ones I did was on modeling an IPv4 address in C++. Below is the full implementation; it's also on Github. Any suggestions? My goal is to make this as modern as possible and I'm not sure if there are some C++17 features I'm not taking advantage of.

ip_address.h

#pragma once

#include <array>
#include <string>
#include <string_view>
#include <stdint.h>

namespace ip
{
    /**
     * @brief Thrown when there is an invalid ip address passed via
     * string.
     */
    class invalid_format_exception : public std::exception
    {
        std::string invalid_format_;
    public:
        invalid_format_exception(const std::string &invalid_format);
        char const* what() const override;
    };

    /**
     * Class that models a IPv4 address. 
     */
    class address
    {
    public:

#pragma region Type definitions
        using value_type = uint8_t;
        using reference = value_type & ;
        using pointer = value_type * ;
        using iterator = std::array<value_type, 4>::iterator;
        using const_iterator = std::array<value_type, 4>::const_iterator;
        using reverse_iterator = std::array<value_type, 4>::reverse_iterator;
        using const_reverse_iterator = std::array<value_type, 4>::const_reverse_iterator;
        using size_type = std::array<value_type, 4>::size_type;
#pragma endregion

        /**
         * @brief Create an IP address representation from the 
         * four parts of the address definition. 
         * @param first the first part of the address 
         * @param second the second part of the address
         * @param third the third part of the address.
         * @param fourth the fourth part of the address.
         * @details Example:
         * @code
         * ip::address addr(127, 0, 0, 1);
         * @endcode
         */
        address(const value_type& first, const value_type &second,
            const value_type &third, const value_type& fourth);

        /**
         * @brief Create an IP address representaiton from an
         * array.
         * @param data the data array. 
         * @details Example:
         * @code
         * ip::address addr = {127, 0, 0, 1};
         * @endcode
         */
        address(const std::array<unsigned char, 4> &data);

        /**
         * @brief Create an IP adderss representation from a 
         * unsigned 32 bit integer.
         * @param value the integer representation of an IP address.
         */
        explicit address(const uint32_t &value);

        /**
         * @brief Implicit conversion to an unsigned 32 bit integer.
         */
        uint32_t operator()() const;

        /**
         * @brief Access operator.
         * @param index the index to access. 
         */
        reference operator[](const int &index) noexcept(false);

        /**
         * @brief Const version of the access operator.
         */
        value_type operator[](const int &index) const noexcept(false);

        /**
         * @brief Prefix increment operator.
         */
        void operator++();

        /**
         * @brief Postfix increment operator.
         */
        ::ip::address& operator++(int);

        /**
         * @brief Prefix decrement operator.
         */
        void operator--();

        /**
         * @brief Prefix decrement operator.
         */
        ::ip::address& operator--(int);

        iterator begin();
        iterator end();
        const_iterator begin() const;
        const_iterator end() const;
    private:
        std::array<value_type, 4> data_;
    };

    bool operator<(const ip::address &first, const ip::address &second);
    bool operator==(const ip::address &first, const ip::address &second);
    std::ostream& operator<<(std::ostream& output, const ip::address &address);
    address from_string(const std::string &view);
    std::string to_string(const address& address);
}

ip_address.cpp

#include <ip_address.h>

#include <iterator>
#include <iostream>
#include <sstream>
#include <regex>
#include <vector>
#include <string>

#pragma region Utilities
template<typename Output>
void split(const std::string &s, char delim, Output result) {
    std::stringstream ss(s);
    std::string item;
    while (std::getline(ss, item, delim)) {
        *(result++) = item;
    }
}

std::vector<std::string> split(const std::string &s, char delim) {
    std::vector<std::string> elems;
    split(s, delim, std::back_inserter(elems));
    return elems;
}

#pragma endregion

ip::invalid_format_exception::invalid_format_exception(const std::string& invalid_format)
    : invalid_format_(invalid_format)
{
}

char const* ip::invalid_format_exception::what() const
{
    std::ostringstream oss;
    oss << "Invalid IP address format: " << invalid_format_;
    return oss.str().c_str();
}

ip::address::address(const value_type & first, const value_type & second, const value_type & third, const value_type & fourth)
{
    data_[0] = first;
    data_[1] = second;
    data_[2] = third;
    data_[3] = fourth;
}

ip::address::address(const std::array<unsigned char, 4>& data)
{
    data_ = data;
}

ip::address::address(const uint32_t& value)
{
    data_[0] = value >> 24 & 0xFF;
    data_[1] = value >> 16 & 0xFF;
    data_[2] = value >> 8 & 0xFF;
    data_[3] = value & 0xFF;
}

uint32_t ip::address::operator()() const
{
    const uint32_t value = data_[0] << 24 | data_[1] << 16 | data_[2] << 8 | data_[3];
    return value;
}

ip::address::reference ip::address::operator[](const int& index)
{
    return data_.at(index);
}

ip::address::value_type ip::address::operator[](const int& index) const
{
    return data_.at(index);
}

void ip::address::operator++()
{
    auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)
    {
        return data < 255;
    });

    if(location != std::rend(data_))
    {
        const auto r_index = std::distance(data_.rbegin(), location);
        auto index = 4 - r_index - 1;
        data_[index]++;
    }
}

::ip::address& ip::address::operator++(int)
{
    auto result(*this);
    ++(*this);
    return result;
}

void ip::address::operator--()
{
    auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)
    {
        return data < 255;
    });

    if (location != std::rend(data_))
    {
        const auto r_index = std::distance(data_.rbegin(), location);
        auto index = 4 - r_index - 1;
        data_[index]--;
    }
}

::ip::address& ip::address::operator--(int)
{
    auto result(*this);
    --(*this);
    return result;
}

ip::address::iterator ip::address::begin()
{
    return data_.begin();
}

ip::address::const_iterator ip::address::end() const
{
    return data_.end();
}

bool ip::operator<(const ip::address& first, const ip::address& second)
{
    return (uint32_t)first() < (uint32_t)second();
}

bool ip::operator==(const ip::address& first, const ip::address& second)
{
    return (uint32_t)first() == (uint32_t) second();
}

ip::address::const_iterator ip::address::begin() const
{
    return data_.begin();
}

ip::address::iterator ip::address::end()
{
    return data_.end();
}

std::ostream& ip::operator<<(std::ostream& output, const ip::address& address)
{
    std::copy(address.begin(), address.end()-1, 
        std::ostream_iterator<short>(output, "."));
    output << +address[3];
    return output;
}

ip::address ip::from_string(const std::string &view)
{
    auto parts = split(view, '.');
    if (parts.size() != 4)
    {
        throw invalid_format_exception(view);
    }

    return {
        (ip::address::value_type)std::stoi(parts[0]),
        (ip::address::value_type)std::stoi(parts[1]),
        (ip::address::value_type)std::stoi(parts[2]),
        (ip::address::value_type)std::stoi(parts[3])
    };
}

std::string ip::to_string(const address& address)
{
    std::ostringstream string_stream;
    string_stream << address;
    return string_stream.str();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ networking-ts has an excellent reference implementation: open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4771.pdf#page179 \$\endgroup\$ – sudo rm -rf slash Apr 5 at 11:41
  • 1
    \$\begingroup\$ Mod Note: Please do not use comments for extended conversations. Comments are intended to resolve unclarities about the post. Use Code Review Chat for extended conversations and in-depth discussions. If you want to critique the code presented in the question, please do so in an answer. Thanks! \$\endgroup\$ – Vogel612 Apr 6 at 16:04
  • \$\begingroup\$ Remember that an ipv4 is not only four numbers separated by 3 dots. 1 = 0.0.0.1, 1.2=1.0.0.2, 1.2.3 = 1.2.0.3, 192.168.0.1 = 3232235521 = 0xC0A80001 \$\endgroup\$ – Lenne Apr 6 at 23:50
  • \$\begingroup\$ @Lenne Yes, but one of the requirements was also that the address be human readable when printed to a stream. \$\endgroup\$ – Developer Paul Apr 7 at 15:18
  • 1
    \$\begingroup\$ @DeveloperPaul, All formats are human-readable ;-). Anyway, your should be able to read all the formats, regardless of what format you want to use for output. \$\endgroup\$ – Lenne Apr 9 at 13:30
34
\$\begingroup\$

I think it's very strange that you provide iterators and an operator[] for an IP address. Generally speaking, IP addresses are not considered to be "iterable"; an IP address is just a single address. If you were modeling a subnet mask, like 127.0.0.0/8, then it might make sense to model it as a range of addresses; but if you're modeling just a single address, I don't think it is appropriate at all to model it as a range of octets. What benefit do you gain from that? IMHO: none. None benefit.


As 1201ProgramAlarm already said, your increment and decrement operators' signatures are a bit screwed up (essentially, backwards). Plus:

::ip::address& ip::address::operator++(int)
{
    auto result(*this);
    ++(*this);
    return result;
}

This one should also have given you a compiler warning (assuming you use any mainstream compiler, such as GCC, Clang, or MSVC). Step number one when writing C++ is always to compile with -W -Wall -Wextra and fix all the warnings prior to publishing your code. The compiler warnings are usually telling you about bugs in your code; and even when they're not technically bugs, you should still fix the warnings, so that none of your coworkers have to read the warnings ever again. Clean code is friendly code!


ip::address::iterator ip::address::begin()
{
    return data_.begin();
}

ip::address::const_iterator ip::address::end() const
{
    return data_.end();
}

It is super weird to me that you define these member functions in the order "nonconst begin, const end, const begin, nonconst end." That's harmless, but it's just weird. Also, I recommend defining these functions directly in-line in the body of the class. They're one-liners. You waste space (and thus, waste the reader's time) by defining them out-of-line. That is, I'd write:

    iterator begin() { return data_.begin(); }
    iterator end() { return data_.end(); }
    const_iterator begin() const { return data_.begin(); }
    const_iterator end() const { return data_.end(); }
private:
    std::array<value_type, 4> data_;

Also, all four of these methods should probably be declared noexcept.


Overloaded comparison operators should always be defined in-line in the body of the class, using the "hidden friend" (a.k.a. "ADL friend," a.k.a. "Barton-Nackman") trick. That is, instead of

class address { ... };

bool operator<(const ip::address &first, const ip::address &second);

bool ip::operator<(const ip::address& first, const ip::address& second)
{
    return (uint32_t)first() < (uint32_t)second();
}

you should write simply

class address {
    // ...

    friend bool operator<(const address& a, const address& b) {
        return uint32_t(a()) < uint32_t(b());
    }
};

Notice that I switched your type-casts from C style to constructor-style, a.k.a. "Python style," just for the heck of it. I find the fewer parentheses the easier it is to read. Also, I switched the verbose first and second to simply a and b: we don't need long names for these extremely locally scoped variables.

But wait, there's more! I initially assumed that first() was a typo — but it's not! You actually declared an overloaded operator():

    /**
     * @brief Implicit conversion to an unsigned 32 bit integer.
     */
    uint32_t operator()() const;

Why on earth is this an overloaded function-call operator instead of a conversion operator? Worse, why is this any kind of operator at all, when you already went out of your way to declare a free function ip::to_string(const address&)? Why is the conversion to uint32_t not implemented as ip::to_uint32(const address&)?

Consistency is important. Also, compatibility with the rest of the language is important. When you overload operator(), you're making ip::address "callable," which means you're enabling your clients to write things like

ip::address myAddress(127, 0, 0, 1);
std::function<int()> f = myAddress;  // !!
assert(f() == 0x7F000001);

Just as with the iterator/range-of-octets business, this functionality strikes me as fundamentally not what an IP address ought to be about. IP addresses aren't ranges, and IP addresses aren't callables. They should be just addresses. To the extent that your ip::address is anything other than just an address, you have actually failed in your stated goal of "modeling an IP address"!


Your operator<< should also be defined in-line.

Anytime you provide operator==, you should also provide operator!= — the language doesn't (yet) provide it for you automatically.

Anytime you provide operator<, you should also provide operator<=, >, and >= — the language doesn't (yet) provide these for you automatically. (But in C++2a you'll have operator<=> to play with!)


void ip::address::operator++()
{
    auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)
    {
        return data < 255;
    });

    if(location != std::rend(data_))
    {
        const auto r_index = std::distance(data_.rbegin(), location);
        auto index = 4 - r_index - 1;
        data_[index]++;
    }
}

It's odd that you write data_.rend() in one place and std::rend(data_) in the other. I recommend the former in both cases, simply because it's shorter.

However, doesn't this code "increment" address(0, 0, 0, 255) to address(0, 0, 1, 255) instead of to address(0, 0, 1, 0)? If so, oops! IMO the clearest and simplest way to write this "odometer algorithm" is simply

address& operator++() noexcept
{
    if (++data_[3] == 0) {
        if (++data_[2] == 0) {
            if (++data_[1] == 0) {
                ++data_[0];
            }
        }
    }
    return *this;
}

Short and sweet. Arguably it's overly complicated and "clever" — but look what it's replacing! What it's replacing uses multiple STL algorithms, is three lines longer, and (AFAICT) doesn't even work. So feel free to reduce the "cleverness" of my proposed code even further, if you can; regardless, I claim it's an improvement over the original.

\$\endgroup\$
  • 2
    \$\begingroup\$ Good point about compiling with warning and fixing said warnings. Also the order of definition for the "begin()" and "end()" methods wasn't on purpose. They are supposed to be in the same order as the declaration. Why should "overloaded comparison operators always be defined in-line in the body of the class"? This seems like conjecture to me and I don't see the real value in bloating the class with something that can easily be left outside the class. I do agree with creating a ip::to_uint32t() function so good point there and I overall agree with your comments about consistency. \$\endgroup\$ – Developer Paul Apr 5 at 12:01
  • \$\begingroup\$ "I find the fewer parentheses the easier it is to read" — haven't you just increased nesting level of parentheses, leaving total count of them unchanged? \$\endgroup\$ – Ruslan Apr 5 at 12:12
  • 2
    \$\begingroup\$ Something else to consider: An address is a small trivial type. Thus, passing by constant reference instead of value is a pessimisation. \$\endgroup\$ – Deduplicator Apr 6 at 0:05
13
\$\begingroup\$

You storing the value in std::array<value_type, 4> which is fine. But if you change your mind on the storage type you have to change this in like 10 places. To make this easier it is a good idea to abstract the storage type and then use this storage type in all places.

    using Storage                = std::array<value_type, 4>;
    using iterator               = Storage::iterator;
    using const_iterator         = Storage::const_iterator;
    using reverse_iterator       = Storage::reverse_iterator;
    using const_reverse_iterator = Storage::const_reverse_iterator;
    using size_type              = Storage::size_type;

Now if you change the underlying storage type you only have to change it in one place.


What does it mean to increment/decrement an ip address?

    /**
     * @brief Prefix increment operator.
     */
    void operator++();

What scenario does this make sense?


If there is a test for equality:

bool operator==(const ip::address &first, const ip::address &second);

Then I would expect a test for inequality.


If there is an output operator:

std::ostream& operator<<(std::ostream& output, const ip::address &address);

Then I would expect an input operator.


The standard exceptions (except std::exception itself) already implement what(). You should inherit from one of these rather than std::exception (probably std::runtime_error.

class invalid_format_exception : public std::exception
{
    std::string invalid_format_;
public:
    invalid_format_exception(const std::string &invalid_format);
    char const* what() const override;
};

This becomes:

struct invalid_format_exception: std::runtime_error
{
    using std::runtime_error::runtime_error;  // Pull runtime_error constructor into this class.
};

Are you sure that the IP address is always stored in big endian form?

data_[0] = value >> 24 & 0xFF;
data_[1] = value >> 16 & 0xFF;
data_[2] = value >> 8 & 0xFF;
data_[3] = value & 0xFF;

I would double check and also add a big comment that that is what you expect.


The increment operator looks complicated.
I think it can really be simplified by using some existing functions you have identified.

void ip::address::operator++()
{
    uint32_t value = (*this);    // convert to 32 bit number
    ++value;                     // Add 1
    (*this) = address(value);    // convert back to address and copy/move
}

Functions that simply forward calls just put them in the class and forget about them. There is nothing to maintain and it need not take up multiple lines in the source file:

ip::address::iterator ip::address::begin()
{
    return data_.begin();
}

ip::address::const_iterator ip::address::end() const
{
    return data_.end();
}

// I would just do the following the header:


    iterator       begin()        {return data_.begin();}
    iterator       end()          {return data_.end();}
    const_iterator begin() const  {return data_.begin();}
    const_iterator end()   const  {return data_.end();}

You are of course missing a few:

    const_iterator   cbegin() const {return data_.cbegin();}
    reverse_iterator rbegin()       {return data_.rbegin();}

     // You can add the end() versions.
\$\endgroup\$
  • \$\begingroup\$ "What does it mean to increment/decrement an ip address?" It means exactly what it sounds like, no? The main purpose was to be able to enumerate a number of addresses in a given range. I didn't mention this in the OP, but it was another part of the challenge. Good point about the storage type, that will definitely be useful. As far as putting the defs of begin(), end() and the like in the header I disagree with that sentiment. I do not like mixing and matching where functions are defined. They will either be all in the header, or all in the source file; not both. \$\endgroup\$ – Developer Paul Apr 5 at 12:06
  • 1
    \$\begingroup\$ @DeveloperPaul: It should not matter were the definitions are. All good development tools will automatically jump to definition when asked. I use vi and it still jumps to the function definition when I ask without me knowing where the file is. \$\endgroup\$ – Martin York Apr 5 at 16:42
  • 1
    \$\begingroup\$ It means exactly what it sounds like, no? Sure but why. This is not a property of an address. Consecutive addresses have no relationship. So this should not be in the address class. You could put it in a helper class that allows you to scan addresses but it should not be part of the address class. \$\endgroup\$ – Martin York Apr 5 at 16:43
  • \$\begingroup\$ I see your point now. Seems like I've included quite a bit in the address class that doesn't need to be there. \$\endgroup\$ – Developer Paul Apr 5 at 18:06
9
\$\begingroup\$

You're passing fundamental types by const reference. These are better off just being passed by value. So you'd get things like

explicit address(uint32_t value);
reference operator[](int index) noexcept(false);

Your prefix increment and decrement operators should return a reference to the incremented value.

address &operator++() { /* ... */ return *this; }
address &operator--() { /* ... */ return *this; }

This will allow expressions like addr = ++other_addr;. (Note that, since you're in the address class, you can just name the class, you don't need to specify scope with ::ip::address).

Your postfix increment and decrement operators have a bug, because they return a reference to a local variable. The return types should be a value.

address operator++(int);
address operator--(int);

For readability and clarity, expressions mixing shifts and bit masking should use parentheses:

data_[0] = (value >> 24) & 0xFF;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.