Modeling an IPv4 Address





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}







14












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









share|improve this question









New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$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
    yesterday






  • 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
    13 hours ago












  • $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
    5 hours ago




















14












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









share|improve this question









New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$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
    yesterday






  • 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
    13 hours ago












  • $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
    5 hours ago
















14












14








14





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









share|improve this question









New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$endgroup$




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






c++ c++11 ip-address






share|improve this question









New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











share|improve this question









New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









share|improve this question




share|improve this question








edited 6 hours ago









Martin Schröder

247518




247518






New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









asked 2 days ago









Developer PaulDeveloper Paul

20116




20116




New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.





New contributor





Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.






Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.








  • 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
    yesterday






  • 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
    13 hours ago












  • $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
    5 hours ago
















  • 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
    yesterday






  • 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
    13 hours ago












  • $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
    5 hours ago










1




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
yesterday




$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
yesterday




1




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
13 hours ago






$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
13 hours ago














$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
5 hours ago






$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
5 hours ago












3 Answers
3






active

oldest

votes


















30












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






share|improve this answer









$endgroup$









  • 1




    $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
    yesterday












  • $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
    yesterday






  • 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
    yesterday



















12












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





share|improve this answer









$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
    yesterday










  • $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
    yesterday










  • $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
    yesterday










  • $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
    yesterday



















8












$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;





share|improve this answer











$endgroup$














    Your Answer





    StackExchange.ifUsing("editor", function () {
    return StackExchange.using("mathjaxEditing", function () {
    StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
    StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
    });
    });
    }, "mathjax-editing");

    StackExchange.ifUsing("editor", function () {
    StackExchange.using("externalEditor", function () {
    StackExchange.using("snippets", function () {
    StackExchange.snippets.init();
    });
    });
    }, "code-snippets");

    StackExchange.ready(function() {
    var channelOptions = {
    tags: "".split(" "),
    id: "196"
    };
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function() {
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled) {
    StackExchange.using("snippets", function() {
    createEditor();
    });
    }
    else {
    createEditor();
    }
    });

    function createEditor() {
    StackExchange.prepareEditor({
    heartbeatType: 'answer',
    autoActivateHeartbeat: false,
    convertImagesToLinks: false,
    noModals: true,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    imageUploader: {
    brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
    contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
    allowUrls: true
    },
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    });


    }
    });






    Developer Paul is a new contributor. Be nice, and check out our Code of Conduct.










    draft saved

    draft discarded


















    StackExchange.ready(
    function () {
    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216894%2fmodeling-an-ipv4-address%23new-answer', 'question_page');
    }
    );

    Post as a guest















    Required, but never shown

























    3 Answers
    3






    active

    oldest

    votes








    3 Answers
    3






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes









    30












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






    share|improve this answer









    $endgroup$









    • 1




      $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
      yesterday












    • $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
      yesterday






    • 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
      yesterday
















    30












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






    share|improve this answer









    $endgroup$









    • 1




      $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
      yesterday












    • $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
      yesterday






    • 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
      yesterday














    30












    30








    30





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






    share|improve this answer









    $endgroup$



    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.







    share|improve this answer












    share|improve this answer



    share|improve this answer










    answered 2 days ago









    QuuxplusoneQuuxplusone

    13.3k12165




    13.3k12165








    • 1




      $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
      yesterday












    • $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
      yesterday






    • 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
      yesterday














    • 1




      $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
      yesterday












    • $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
      yesterday






    • 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
      yesterday








    1




    1




    $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
    yesterday






    $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
    yesterday














    $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
    yesterday




    $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
    yesterday




    2




    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
    yesterday




    $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
    yesterday













    12












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





    share|improve this answer









    $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
      yesterday










    • $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
      yesterday










    • $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
      yesterday










    • $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
      yesterday
















    12












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





    share|improve this answer









    $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
      yesterday










    • $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
      yesterday










    • $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
      yesterday










    • $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
      yesterday














    12












    12








    12





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





    share|improve this answer









    $endgroup$



    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.






    share|improve this answer












    share|improve this answer



    share|improve this answer










    answered 2 days ago









    Martin YorkMartin York

    74.4k488273




    74.4k488273












    • $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
      yesterday










    • $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
      yesterday










    • $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
      yesterday










    • $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
      yesterday


















    • $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
      yesterday










    • $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
      yesterday










    • $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
      yesterday










    • $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
      yesterday
















    $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
    yesterday




    $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
    yesterday












    $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
    yesterday




    $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
    yesterday












    $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
    yesterday




    $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
    yesterday












    $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
    yesterday




    $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
    yesterday











    8












    $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;





    share|improve this answer











    $endgroup$


















      8












      $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;





      share|improve this answer











      $endgroup$
















        8












        8








        8





        $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;





        share|improve this answer











        $endgroup$



        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;






        share|improve this answer














        share|improve this answer



        share|improve this answer








        edited 2 days ago

























        answered 2 days ago









        1201ProgramAlarm1201ProgramAlarm

        3,6932925




        3,6932925






















            Developer Paul is a new contributor. Be nice, and check out our Code of Conduct.










            draft saved

            draft discarded


















            Developer Paul is a new contributor. Be nice, and check out our Code of Conduct.













            Developer Paul is a new contributor. Be nice, and check out our Code of Conduct.












            Developer Paul is a new contributor. Be nice, and check out our Code of Conduct.
















            Thanks for contributing an answer to Code Review Stack Exchange!


            • Please be sure to answer the question. Provide details and share your research!

            But avoid



            • Asking for help, clarification, or responding to other answers.

            • Making statements based on opinion; back them up with references or personal experience.


            Use MathJax to format equations. MathJax reference.


            To learn more, see our tips on writing great answers.




            draft saved


            draft discarded














            StackExchange.ready(
            function () {
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216894%2fmodeling-an-ipv4-address%23new-answer', 'question_page');
            }
            );

            Post as a guest















            Required, but never shown





















































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown

































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown







            Popular posts from this blog

            Knooppunt Holsloot

            Altaar (religie)

            Gregoriusmis