4
\$\begingroup\$

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-9}{0-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
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

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

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