Crthuky

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 

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();

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.

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.

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).