C++ string Formatter Again Part-2

Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
4
down vote
favorite
Previously asked here.
The code is now available on GitHub.
Since the previous review I have added unit tests.
Since it is big it will come in a couple of parts.
Part 1 | Part 2 | Part 3 | Part 4
Part 2
This is a simple helper object. It stores the result of parsing the string in an object. This way we don't need to re-parse the string for information.
FormatInfo.h
#ifndef THORSANVIL_IOUTIL_FORMAT_INFO_H
#define THORSANVIL_IOUTIL_FORMAT_INFO_H
#include <iostream>
#include <typeinfo>
namespace ThorsAnvil::IOUtil
// Enum representing the Length, specifier and type information provided by the format string
#pragma vera-pushoff
// printf doc http://www.cplusplus.com/reference/cstdio/printf/
enum class Length none, hh, h, l, ll, j, z, t, L; // Use the same values as in printf documentation so it is easy to lookup
enum class Specifier d, i, u, o, x, X, f, F, e, E, g, G, a, A, c, s, p, n; // Use the same values as in printf documentation so it is easy to lookup
enum class Type Int, UInt, Float, Char, String, Pointer, Count; // The type we are expecting
enum class Dynamic None, Width, Precision, Both; // If width and/or precision are specified by parameter
#pragma vera-pop
using AllowedType = std::pair<std::type_info const*, int>;
struct FormatInfo
// Formatter has the text representation:
// %<Flag>*<Width>?[.<Precision>]?<Length>?<Specifier>
//
// Flag: - + <space> # 0
// Width: 1-90-9*
// Precision: 0-9*
// Length: hh h l ll j z t L
// Specifier: d i u o x X f F e E g G a A c s p n
// Flags
bool leftJustify; // - Left-justify within the given field width; Right justification is the default (see width sub-specifier).
bool forceSign; // + Forces to precede the result with a plus or minus sign (+ or -) even for positive numbers. By default, only negative numbers are preceded with a - sign.
bool forceSignWidth; // (space) If no sign is going to be written, a blank space is inserted before the value.
bool prefixType; // # Used with o, x or X specifiers the value is preceded with 0, 0x or 0X respectively for values different than zero.
// Used with a, A, e, E, f, F, g or G it forces the written output to contain a decimal point even if no more digits follow. By default, if no digits follow, no decimal point is written.
bool leftPad; // 0 Left-pads the number with zeroes (0) instead of spaces when padding is specified (see width sub-specifier).
// Width and precision of the formatter.
std::size_t width;
std::size_t precision;
Length length;
Specifier specifier;
// Pre-calculate a some values based on the format string
Type type; // Calculated based on length/specifier
Dynamic useDynamicSize; // Are width/precision dynamically specified.
AllowedType expectedType; // Type info we expect the next argument to be based on length/specifier
std::ios_base::fmtflags format; // The format flags that will be applied to stream before the next argument
;
#endif
c++ formatting serialization stream
add a comment |Â
up vote
4
down vote
favorite
Previously asked here.
The code is now available on GitHub.
Since the previous review I have added unit tests.
Since it is big it will come in a couple of parts.
Part 1 | Part 2 | Part 3 | Part 4
Part 2
This is a simple helper object. It stores the result of parsing the string in an object. This way we don't need to re-parse the string for information.
FormatInfo.h
#ifndef THORSANVIL_IOUTIL_FORMAT_INFO_H
#define THORSANVIL_IOUTIL_FORMAT_INFO_H
#include <iostream>
#include <typeinfo>
namespace ThorsAnvil::IOUtil
// Enum representing the Length, specifier and type information provided by the format string
#pragma vera-pushoff
// printf doc http://www.cplusplus.com/reference/cstdio/printf/
enum class Length none, hh, h, l, ll, j, z, t, L; // Use the same values as in printf documentation so it is easy to lookup
enum class Specifier d, i, u, o, x, X, f, F, e, E, g, G, a, A, c, s, p, n; // Use the same values as in printf documentation so it is easy to lookup
enum class Type Int, UInt, Float, Char, String, Pointer, Count; // The type we are expecting
enum class Dynamic None, Width, Precision, Both; // If width and/or precision are specified by parameter
#pragma vera-pop
using AllowedType = std::pair<std::type_info const*, int>;
struct FormatInfo
// Formatter has the text representation:
// %<Flag>*<Width>?[.<Precision>]?<Length>?<Specifier>
//
// Flag: - + <space> # 0
// Width: 1-90-9*
// Precision: 0-9*
// Length: hh h l ll j z t L
// Specifier: d i u o x X f F e E g G a A c s p n
// Flags
bool leftJustify; // - Left-justify within the given field width; Right justification is the default (see width sub-specifier).
bool forceSign; // + Forces to precede the result with a plus or minus sign (+ or -) even for positive numbers. By default, only negative numbers are preceded with a - sign.
bool forceSignWidth; // (space) If no sign is going to be written, a blank space is inserted before the value.
bool prefixType; // # Used with o, x or X specifiers the value is preceded with 0, 0x or 0X respectively for values different than zero.
// Used with a, A, e, E, f, F, g or G it forces the written output to contain a decimal point even if no more digits follow. By default, if no digits follow, no decimal point is written.
bool leftPad; // 0 Left-pads the number with zeroes (0) instead of spaces when padding is specified (see width sub-specifier).
// Width and precision of the formatter.
std::size_t width;
std::size_t precision;
Length length;
Specifier specifier;
// Pre-calculate a some values based on the format string
Type type; // Calculated based on length/specifier
Dynamic useDynamicSize; // Are width/precision dynamically specified.
AllowedType expectedType; // Type info we expect the next argument to be based on length/specifier
std::ios_base::fmtflags format; // The format flags that will be applied to stream before the next argument
;
#endif
c++ formatting serialization stream
add a comment |Â
up vote
4
down vote
favorite
up vote
4
down vote
favorite
Previously asked here.
The code is now available on GitHub.
Since the previous review I have added unit tests.
Since it is big it will come in a couple of parts.
Part 1 | Part 2 | Part 3 | Part 4
Part 2
This is a simple helper object. It stores the result of parsing the string in an object. This way we don't need to re-parse the string for information.
FormatInfo.h
#ifndef THORSANVIL_IOUTIL_FORMAT_INFO_H
#define THORSANVIL_IOUTIL_FORMAT_INFO_H
#include <iostream>
#include <typeinfo>
namespace ThorsAnvil::IOUtil
// Enum representing the Length, specifier and type information provided by the format string
#pragma vera-pushoff
// printf doc http://www.cplusplus.com/reference/cstdio/printf/
enum class Length none, hh, h, l, ll, j, z, t, L; // Use the same values as in printf documentation so it is easy to lookup
enum class Specifier d, i, u, o, x, X, f, F, e, E, g, G, a, A, c, s, p, n; // Use the same values as in printf documentation so it is easy to lookup
enum class Type Int, UInt, Float, Char, String, Pointer, Count; // The type we are expecting
enum class Dynamic None, Width, Precision, Both; // If width and/or precision are specified by parameter
#pragma vera-pop
using AllowedType = std::pair<std::type_info const*, int>;
struct FormatInfo
// Formatter has the text representation:
// %<Flag>*<Width>?[.<Precision>]?<Length>?<Specifier>
//
// Flag: - + <space> # 0
// Width: 1-90-9*
// Precision: 0-9*
// Length: hh h l ll j z t L
// Specifier: d i u o x X f F e E g G a A c s p n
// Flags
bool leftJustify; // - Left-justify within the given field width; Right justification is the default (see width sub-specifier).
bool forceSign; // + Forces to precede the result with a plus or minus sign (+ or -) even for positive numbers. By default, only negative numbers are preceded with a - sign.
bool forceSignWidth; // (space) If no sign is going to be written, a blank space is inserted before the value.
bool prefixType; // # Used with o, x or X specifiers the value is preceded with 0, 0x or 0X respectively for values different than zero.
// Used with a, A, e, E, f, F, g or G it forces the written output to contain a decimal point even if no more digits follow. By default, if no digits follow, no decimal point is written.
bool leftPad; // 0 Left-pads the number with zeroes (0) instead of spaces when padding is specified (see width sub-specifier).
// Width and precision of the formatter.
std::size_t width;
std::size_t precision;
Length length;
Specifier specifier;
// Pre-calculate a some values based on the format string
Type type; // Calculated based on length/specifier
Dynamic useDynamicSize; // Are width/precision dynamically specified.
AllowedType expectedType; // Type info we expect the next argument to be based on length/specifier
std::ios_base::fmtflags format; // The format flags that will be applied to stream before the next argument
;
#endif
c++ formatting serialization stream
Previously asked here.
The code is now available on GitHub.
Since the previous review I have added unit tests.
Since it is big it will come in a couple of parts.
Part 1 | Part 2 | Part 3 | Part 4
Part 2
This is a simple helper object. It stores the result of parsing the string in an object. This way we don't need to re-parse the string for information.
FormatInfo.h
#ifndef THORSANVIL_IOUTIL_FORMAT_INFO_H
#define THORSANVIL_IOUTIL_FORMAT_INFO_H
#include <iostream>
#include <typeinfo>
namespace ThorsAnvil::IOUtil
// Enum representing the Length, specifier and type information provided by the format string
#pragma vera-pushoff
// printf doc http://www.cplusplus.com/reference/cstdio/printf/
enum class Length none, hh, h, l, ll, j, z, t, L; // Use the same values as in printf documentation so it is easy to lookup
enum class Specifier d, i, u, o, x, X, f, F, e, E, g, G, a, A, c, s, p, n; // Use the same values as in printf documentation so it is easy to lookup
enum class Type Int, UInt, Float, Char, String, Pointer, Count; // The type we are expecting
enum class Dynamic None, Width, Precision, Both; // If width and/or precision are specified by parameter
#pragma vera-pop
using AllowedType = std::pair<std::type_info const*, int>;
struct FormatInfo
// Formatter has the text representation:
// %<Flag>*<Width>?[.<Precision>]?<Length>?<Specifier>
//
// Flag: - + <space> # 0
// Width: 1-90-9*
// Precision: 0-9*
// Length: hh h l ll j z t L
// Specifier: d i u o x X f F e E g G a A c s p n
// Flags
bool leftJustify; // - Left-justify within the given field width; Right justification is the default (see width sub-specifier).
bool forceSign; // + Forces to precede the result with a plus or minus sign (+ or -) even for positive numbers. By default, only negative numbers are preceded with a - sign.
bool forceSignWidth; // (space) If no sign is going to be written, a blank space is inserted before the value.
bool prefixType; // # Used with o, x or X specifiers the value is preceded with 0, 0x or 0X respectively for values different than zero.
// Used with a, A, e, E, f, F, g or G it forces the written output to contain a decimal point even if no more digits follow. By default, if no digits follow, no decimal point is written.
bool leftPad; // 0 Left-pads the number with zeroes (0) instead of spaces when padding is specified (see width sub-specifier).
// Width and precision of the formatter.
std::size_t width;
std::size_t precision;
Length length;
Specifier specifier;
// Pre-calculate a some values based on the format string
Type type; // Calculated based on length/specifier
Dynamic useDynamicSize; // Are width/precision dynamically specified.
AllowedType expectedType; // Type info we expect the next argument to be based on length/specifier
std::ios_base::fmtflags format; // The format flags that will be applied to stream before the next argument
;
#endif
c++ formatting serialization stream
edited Mar 4 at 19:51
Phrancis
14.6k645137
14.6k645137
asked Mar 4 at 19:19
Martin York
70.9k481244
70.9k481244
add a comment |Â
add a comment |Â
1 Answer
1
active
oldest
votes
up vote
3
down vote
Very little code prompts a very small review. Here are a few things that came to my mind:
Includes
One issue is definitely that you are missing #include <utility> for std::pair, as well as #include <cstddef> (or similar) for std::size_t.
Another thing that might turn out to make some people angry at you is the fact that you #include <iostream>. The reason for this is that this header injects some static initializers into the current compilation unit which initialize the special streams it provides, which in turn leads to binary bloat and a possible performance reduction (take a look at the assembly that your header generates).
On almost all occasions, #include <iostream> can be avoided by including one of several other headers, in you case #include <ios>, which provides std::ios_base.
Ugh, std::type_info
Usually, seeing std::type_info around is setting off some alarm bells for me. The cause for this is not that there is anything inherently wrong with std::type_info, but that most things it could be used for are better expressed through polymorphism or compile time template switching.
From the single file you provide, I cannot possibly tell whether that is the case with your code or not. However, passing a pointer to some std::type_info around together with an int does hint at somewhat dubious practices. Please reevaluate whether the use of std::type_info is really appropriate.
Comments
// Enum representing the Length, specifier and type information provided by the format string
seems borderline unnecessary to me. What the enums represent is already made sufficiently clear by their name, only the information bit linking them to the whole format string functionality is not given in the code (however, since the header is named FormatInfo.h, I would say this information is implied).
// printf doc http://www.cplusplus.com/reference/cstdio/printf/
// Use the same values as in printf documentation so it is easy to lookup
// Use the same values as in printf documentation so it is easy to lookup
// The type we are expecting
// If width and/or precision are specified by parameter
That first comment is bad, because linking to external resources is subject to link rotting. Only do so when the information that is provided there is not easily available anywhere else. In the case of printf parameters this is not the case; most POSIX-compliant systems even have a manpage explaining them thoroughly. Furthermore, the use of printf is so ubiquitous that I'd say it is acceptable to assume users are able to find documentation on their own.
The next two comments are exactly the same and should be coerced into a single commit above the two applying enums.
Both of the last comments do not at a whole lot of valuable information to the code, so I'd say you should omit them.
Structure
FormatInfo seems to contain three separate "blocks" of information/settings. Since these blocks each mark a certain category of information, I would have made nested structs for them. My suggestion would look something akin to this:
struct FormatInfo
struct Flags
bool leftJustify;
bool forceSignWidth;
bool prefixType;
bool leftPad;
;
struct Dimension
std::size_t width;
std::size_t precision;
Length length;
Specifier specifier;
;
struct FormatSettings
Type type;
Dynamic useDynamicSize;
AllowedType expectedType;
std::ios_base::fmtflags format;
;
Flags flags;
Dimension dimension;
FormatSettings formatSettings;
;
which encapsulates the different setting groups better.
Depending on your use case, there is also a point in making the four enums from the top of your file part of FormatInfo if this makes sense from a structural point of view (again, I don't have an overview of your whole project structure).
Apart from the mentioned points, some points of my answer to Part 1 also apply here, for example the point about indentation.
add a comment |Â
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
3
down vote
Very little code prompts a very small review. Here are a few things that came to my mind:
Includes
One issue is definitely that you are missing #include <utility> for std::pair, as well as #include <cstddef> (or similar) for std::size_t.
Another thing that might turn out to make some people angry at you is the fact that you #include <iostream>. The reason for this is that this header injects some static initializers into the current compilation unit which initialize the special streams it provides, which in turn leads to binary bloat and a possible performance reduction (take a look at the assembly that your header generates).
On almost all occasions, #include <iostream> can be avoided by including one of several other headers, in you case #include <ios>, which provides std::ios_base.
Ugh, std::type_info
Usually, seeing std::type_info around is setting off some alarm bells for me. The cause for this is not that there is anything inherently wrong with std::type_info, but that most things it could be used for are better expressed through polymorphism or compile time template switching.
From the single file you provide, I cannot possibly tell whether that is the case with your code or not. However, passing a pointer to some std::type_info around together with an int does hint at somewhat dubious practices. Please reevaluate whether the use of std::type_info is really appropriate.
Comments
// Enum representing the Length, specifier and type information provided by the format string
seems borderline unnecessary to me. What the enums represent is already made sufficiently clear by their name, only the information bit linking them to the whole format string functionality is not given in the code (however, since the header is named FormatInfo.h, I would say this information is implied).
// printf doc http://www.cplusplus.com/reference/cstdio/printf/
// Use the same values as in printf documentation so it is easy to lookup
// Use the same values as in printf documentation so it is easy to lookup
// The type we are expecting
// If width and/or precision are specified by parameter
That first comment is bad, because linking to external resources is subject to link rotting. Only do so when the information that is provided there is not easily available anywhere else. In the case of printf parameters this is not the case; most POSIX-compliant systems even have a manpage explaining them thoroughly. Furthermore, the use of printf is so ubiquitous that I'd say it is acceptable to assume users are able to find documentation on their own.
The next two comments are exactly the same and should be coerced into a single commit above the two applying enums.
Both of the last comments do not at a whole lot of valuable information to the code, so I'd say you should omit them.
Structure
FormatInfo seems to contain three separate "blocks" of information/settings. Since these blocks each mark a certain category of information, I would have made nested structs for them. My suggestion would look something akin to this:
struct FormatInfo
struct Flags
bool leftJustify;
bool forceSignWidth;
bool prefixType;
bool leftPad;
;
struct Dimension
std::size_t width;
std::size_t precision;
Length length;
Specifier specifier;
;
struct FormatSettings
Type type;
Dynamic useDynamicSize;
AllowedType expectedType;
std::ios_base::fmtflags format;
;
Flags flags;
Dimension dimension;
FormatSettings formatSettings;
;
which encapsulates the different setting groups better.
Depending on your use case, there is also a point in making the four enums from the top of your file part of FormatInfo if this makes sense from a structural point of view (again, I don't have an overview of your whole project structure).
Apart from the mentioned points, some points of my answer to Part 1 also apply here, for example the point about indentation.
add a comment |Â
up vote
3
down vote
Very little code prompts a very small review. Here are a few things that came to my mind:
Includes
One issue is definitely that you are missing #include <utility> for std::pair, as well as #include <cstddef> (or similar) for std::size_t.
Another thing that might turn out to make some people angry at you is the fact that you #include <iostream>. The reason for this is that this header injects some static initializers into the current compilation unit which initialize the special streams it provides, which in turn leads to binary bloat and a possible performance reduction (take a look at the assembly that your header generates).
On almost all occasions, #include <iostream> can be avoided by including one of several other headers, in you case #include <ios>, which provides std::ios_base.
Ugh, std::type_info
Usually, seeing std::type_info around is setting off some alarm bells for me. The cause for this is not that there is anything inherently wrong with std::type_info, but that most things it could be used for are better expressed through polymorphism or compile time template switching.
From the single file you provide, I cannot possibly tell whether that is the case with your code or not. However, passing a pointer to some std::type_info around together with an int does hint at somewhat dubious practices. Please reevaluate whether the use of std::type_info is really appropriate.
Comments
// Enum representing the Length, specifier and type information provided by the format string
seems borderline unnecessary to me. What the enums represent is already made sufficiently clear by their name, only the information bit linking them to the whole format string functionality is not given in the code (however, since the header is named FormatInfo.h, I would say this information is implied).
// printf doc http://www.cplusplus.com/reference/cstdio/printf/
// Use the same values as in printf documentation so it is easy to lookup
// Use the same values as in printf documentation so it is easy to lookup
// The type we are expecting
// If width and/or precision are specified by parameter
That first comment is bad, because linking to external resources is subject to link rotting. Only do so when the information that is provided there is not easily available anywhere else. In the case of printf parameters this is not the case; most POSIX-compliant systems even have a manpage explaining them thoroughly. Furthermore, the use of printf is so ubiquitous that I'd say it is acceptable to assume users are able to find documentation on their own.
The next two comments are exactly the same and should be coerced into a single commit above the two applying enums.
Both of the last comments do not at a whole lot of valuable information to the code, so I'd say you should omit them.
Structure
FormatInfo seems to contain three separate "blocks" of information/settings. Since these blocks each mark a certain category of information, I would have made nested structs for them. My suggestion would look something akin to this:
struct FormatInfo
struct Flags
bool leftJustify;
bool forceSignWidth;
bool prefixType;
bool leftPad;
;
struct Dimension
std::size_t width;
std::size_t precision;
Length length;
Specifier specifier;
;
struct FormatSettings
Type type;
Dynamic useDynamicSize;
AllowedType expectedType;
std::ios_base::fmtflags format;
;
Flags flags;
Dimension dimension;
FormatSettings formatSettings;
;
which encapsulates the different setting groups better.
Depending on your use case, there is also a point in making the four enums from the top of your file part of FormatInfo if this makes sense from a structural point of view (again, I don't have an overview of your whole project structure).
Apart from the mentioned points, some points of my answer to Part 1 also apply here, for example the point about indentation.
add a comment |Â
up vote
3
down vote
up vote
3
down vote
Very little code prompts a very small review. Here are a few things that came to my mind:
Includes
One issue is definitely that you are missing #include <utility> for std::pair, as well as #include <cstddef> (or similar) for std::size_t.
Another thing that might turn out to make some people angry at you is the fact that you #include <iostream>. The reason for this is that this header injects some static initializers into the current compilation unit which initialize the special streams it provides, which in turn leads to binary bloat and a possible performance reduction (take a look at the assembly that your header generates).
On almost all occasions, #include <iostream> can be avoided by including one of several other headers, in you case #include <ios>, which provides std::ios_base.
Ugh, std::type_info
Usually, seeing std::type_info around is setting off some alarm bells for me. The cause for this is not that there is anything inherently wrong with std::type_info, but that most things it could be used for are better expressed through polymorphism or compile time template switching.
From the single file you provide, I cannot possibly tell whether that is the case with your code or not. However, passing a pointer to some std::type_info around together with an int does hint at somewhat dubious practices. Please reevaluate whether the use of std::type_info is really appropriate.
Comments
// Enum representing the Length, specifier and type information provided by the format string
seems borderline unnecessary to me. What the enums represent is already made sufficiently clear by their name, only the information bit linking them to the whole format string functionality is not given in the code (however, since the header is named FormatInfo.h, I would say this information is implied).
// printf doc http://www.cplusplus.com/reference/cstdio/printf/
// Use the same values as in printf documentation so it is easy to lookup
// Use the same values as in printf documentation so it is easy to lookup
// The type we are expecting
// If width and/or precision are specified by parameter
That first comment is bad, because linking to external resources is subject to link rotting. Only do so when the information that is provided there is not easily available anywhere else. In the case of printf parameters this is not the case; most POSIX-compliant systems even have a manpage explaining them thoroughly. Furthermore, the use of printf is so ubiquitous that I'd say it is acceptable to assume users are able to find documentation on their own.
The next two comments are exactly the same and should be coerced into a single commit above the two applying enums.
Both of the last comments do not at a whole lot of valuable information to the code, so I'd say you should omit them.
Structure
FormatInfo seems to contain three separate "blocks" of information/settings. Since these blocks each mark a certain category of information, I would have made nested structs for them. My suggestion would look something akin to this:
struct FormatInfo
struct Flags
bool leftJustify;
bool forceSignWidth;
bool prefixType;
bool leftPad;
;
struct Dimension
std::size_t width;
std::size_t precision;
Length length;
Specifier specifier;
;
struct FormatSettings
Type type;
Dynamic useDynamicSize;
AllowedType expectedType;
std::ios_base::fmtflags format;
;
Flags flags;
Dimension dimension;
FormatSettings formatSettings;
;
which encapsulates the different setting groups better.
Depending on your use case, there is also a point in making the four enums from the top of your file part of FormatInfo if this makes sense from a structural point of view (again, I don't have an overview of your whole project structure).
Apart from the mentioned points, some points of my answer to Part 1 also apply here, for example the point about indentation.
Very little code prompts a very small review. Here are a few things that came to my mind:
Includes
One issue is definitely that you are missing #include <utility> for std::pair, as well as #include <cstddef> (or similar) for std::size_t.
Another thing that might turn out to make some people angry at you is the fact that you #include <iostream>. The reason for this is that this header injects some static initializers into the current compilation unit which initialize the special streams it provides, which in turn leads to binary bloat and a possible performance reduction (take a look at the assembly that your header generates).
On almost all occasions, #include <iostream> can be avoided by including one of several other headers, in you case #include <ios>, which provides std::ios_base.
Ugh, std::type_info
Usually, seeing std::type_info around is setting off some alarm bells for me. The cause for this is not that there is anything inherently wrong with std::type_info, but that most things it could be used for are better expressed through polymorphism or compile time template switching.
From the single file you provide, I cannot possibly tell whether that is the case with your code or not. However, passing a pointer to some std::type_info around together with an int does hint at somewhat dubious practices. Please reevaluate whether the use of std::type_info is really appropriate.
Comments
// Enum representing the Length, specifier and type information provided by the format string
seems borderline unnecessary to me. What the enums represent is already made sufficiently clear by their name, only the information bit linking them to the whole format string functionality is not given in the code (however, since the header is named FormatInfo.h, I would say this information is implied).
// printf doc http://www.cplusplus.com/reference/cstdio/printf/
// Use the same values as in printf documentation so it is easy to lookup
// Use the same values as in printf documentation so it is easy to lookup
// The type we are expecting
// If width and/or precision are specified by parameter
That first comment is bad, because linking to external resources is subject to link rotting. Only do so when the information that is provided there is not easily available anywhere else. In the case of printf parameters this is not the case; most POSIX-compliant systems even have a manpage explaining them thoroughly. Furthermore, the use of printf is so ubiquitous that I'd say it is acceptable to assume users are able to find documentation on their own.
The next two comments are exactly the same and should be coerced into a single commit above the two applying enums.
Both of the last comments do not at a whole lot of valuable information to the code, so I'd say you should omit them.
Structure
FormatInfo seems to contain three separate "blocks" of information/settings. Since these blocks each mark a certain category of information, I would have made nested structs for them. My suggestion would look something akin to this:
struct FormatInfo
struct Flags
bool leftJustify;
bool forceSignWidth;
bool prefixType;
bool leftPad;
;
struct Dimension
std::size_t width;
std::size_t precision;
Length length;
Specifier specifier;
;
struct FormatSettings
Type type;
Dynamic useDynamicSize;
AllowedType expectedType;
std::ios_base::fmtflags format;
;
Flags flags;
Dimension dimension;
FormatSettings formatSettings;
;
which encapsulates the different setting groups better.
Depending on your use case, there is also a point in making the four enums from the top of your file part of FormatInfo if this makes sense from a structural point of view (again, I don't have an overview of your whole project structure).
Apart from the mentioned points, some points of my answer to Part 1 also apply here, for example the point about indentation.
edited Mar 6 at 20:12
yuri
3,3862832
3,3862832
answered Mar 5 at 21:02
Ben Steffan
4,85011234
4,85011234
add a comment |Â
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f188810%2fc-string-formatter-again-part-2%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password