5
\$\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 1

This is the interface. It allows you to pass a format string and a bunch of arguments to be formatted. If the result of this is then applied to a stream it will print the value.

Format.h

#ifndef THORSANVIL_IOUTIL_FORMAT_H
#define THORSANVIL_IOUTIL_FORMAT_H

#include "printToStream.h"
#include "Formatter.h"

#include <ostream>
#include <sstream>
#include <string>
#include <vector>
#include <exception>
#include <stdexcept>

namespace ThorsAnvil::IOUtil
{

template<typename... Args>
class Format
{
    std::string                     format;
    std::tuple<Args const&...>      arguments;
    std::vector<std::string>        prefixString;
    std::vector<Formatter>          formater;
    public:
        Format(char const* fmt, Args const&... args)
            : format(fmt)
            , arguments(args...)
        {
            std::size_t count       = sizeof...(args);
            std::size_t pos         = 0;
            Dynamic     dynamicSize = Dynamic::None;
            for (std::size_t loop = 0; loop < count; ++loop)
            {
                // Find the prefix string before the next % sign (this may be empty)
                std::pair<std::string, std::size_t> prefix = getNextPrefix(format, pos, [](std::size_t p){return p == std::string::npos;}, "not enough format");
                pos += prefix.second;
                prefixString.emplace_back(std::move(prefix.first));

                // Now that I have found the %
                // Convert the next part of the string into a Formatter object.
                formater.emplace_back(format.data() + pos, dynamicSize);
                pos         += formater.back().size();

                // Take into account the Dynamic Width/Precesion prevent the format from being read.
                // Note we can have Width/Precision or Both.
                Dynamic     newDynamic  = formater.back().isDynamicSize();
                dynamicSize = (newDynamic == Dynamic::Precision && dynamicSize == Dynamic::Width) ? Dynamic::Both : newDynamic;
            }
            // After the last formatter check for a last fixed string (this may be empty)
            // But there better not be any more % signs as we don't have any parameters left for them.
            std::pair<std::string, std::size_t> prefix = getNextPrefix(format, pos, [](std::size_t p){return p != std::string::npos;}, "too many format");
            pos += prefix.second;
            prefixString.emplace_back(std::move(prefix.first));
        }
        // Using the operator<< is the same as calling print on the object.
        friend std::ostream& operator<<(std::ostream& s, Format const& format)
        {
            format.print(s);
            return s;
        }
        void print(std::ostream& s) const
        {
            doPrint(s, std::make_index_sequence<sizeof...(Args)>());
        }
    private:
        // This finds the next '%' taking into account that %% is a not a scan token.
        std::pair<std::string, std::size_t> getNextPrefix(std::string const&, std::size_t pos, std::function<bool(std::size_t)>&& test, char const* mes)
        {
            std::string prefix;
            std::size_t extra = 0;
            std::size_t nextFormatter = format.find('%', pos);
            while (nextFormatter != std::string::npos && format[nextFormatter + 1] == '%')
            {
                nextFormatter = format.find('%', nextFormatter + 2);
            }
            prefix += format.substr(pos, nextFormatter - pos);
            prefix.erase(std::remove_if(std::begin(prefix), std::end(prefix), [first = false](char val) mutable
            {
                if (val == '%')
                {
                    first = !first;
                    return !first;
                }
                return false;
            }), std::end(prefix));
            if (test(nextFormatter))
            {
                std::stringstream message;
                message << "Invalid Format: " << mes << " specifiers for provided arguments";
                throw std::invalid_argument(message.str());
            }
            return {prefix, prefix.size() + extra};
        }
        // For each argument we pass
        // Pass a prefix string (might be empty) the formatter then the argument.
        template<std::size_t I>
        std::ostream& printValue(std::ostream& s) const
        {
            return s << prefixString[I] << formater[I] << std::get<I>(arguments);
        }

        // Print all the values in order by calling printValue() on each parameter.
        // Then print the final fixed string (might be empty)
        template<std::size_t... I>
        void doPrint(std::ostream& s, std::index_sequence<I...> const&) const
        {
            std::ostream* ignore[] = {&printValue<I>(s)...};
            s << prefixString.back();
        }

};

template<typename... Args>
Format<Args...> make_format(char const* fmt, Args const&... args)
{
    return Format<Args...>(fmt, args...);
}

}

#endif

Usage:

int main()
{
    std::cout << make_format("Test One\n");
    std::cout << make_format("Test Two   %d\n", 12);
    std::cout << make_format("Test Three %d %f\n", 12, 4.56);
    std::cout << make_format("Test Four  %d %d\n", 12, 4.56); // Should throw
}
\$\endgroup\$
  • \$\begingroup\$ Apart from the fact that it isn't part of the standard, are there reasons not to use #pragma once instead of include guards? \$\endgroup\$ – yuri Mar 4 '18 at 19:27
  • 1
    \$\begingroup\$ @yuri not all my compiler support them. \$\endgroup\$ – Martin York Mar 4 '18 at 19:30
  • \$\begingroup\$ The linked unit tests have a comment saying "see the README" but there's no obvious README - I guess it needs a little modification having been lifted from somewhere else? \$\endgroup\$ – Toby Speight Mar 5 '18 at 10:54
  • \$\begingroup\$ @TobySpeight. Oops. I should fix that. I was trying to keep the whole (C) section intact. Now I need to find where I got it from. \$\endgroup\$ – Martin York Mar 5 '18 at 16:48
7
\$\begingroup\$

First of all, this is remarkably good and clean code! Nevertheless, I do have some things to nitpick on:

Code Formatting

Let's talk about line length. As of right now, you have a (physical) line in your program weighing in at a whopping 160 characters.

First off all, this is easily enough to make the code blocks here on code review have a horizontal scrollbar, which is impractical and not very beautiful.

Second of all, 160 chars is too much for many of us to handle in a typical development environment. Maybe you have a nice, big monitor or two which allow having open four files simultaneously at a line width of 200 or more. However, I certainly don't, and making me work with your program on my 14" diagonal laptop with about 200 characters line width for the whole screen is not very accommodating of you.

Now, the correct maximal line width has been a matter of ongoing debate. I'm an advocate of the good old 80 characters standard, but I'd argue that anything around 100 characters is fine. However, in my opinion, 160 is not; and you are likely to upset some people by sticking to it.

Another issue I see is horizontal spacing. For example, let's take a look at two lines from your code:

pos         += formater.back().size();

Dynamic     newDynamic  = formater.back().isDynamicSize();

Both of these are to be found in the body of the Format constructor. Obviously, you are trying to align you definitions/operators. Even more obviously, you are failing at it. Although both of these lines align with something, what they align with is not all easy to recognize. I don't know whether this is the fault of a tab-width mismatch between the code block and your original code. All I can say is that that indentation looks out-of-place, and left me confused the first time I glossed over your code.

In general, aligning initializers etc. is completely acceptable, but you should refrain from doing it if the alignment reference is not at most one line away. Otherwise, it will just look strange, and overall a programmer's as good as you unworthy.

About char const*

Ah, C-strings. Don't use them unless you definitely have to. Why? Because...

  1. They are inherently unsafe. You have no guarantee that the pointer you receive is a pointer to a valid, zero-terminated string and not to anything else, like an array of binary data, or nowhere at all.
  2. C-Strings are dumb. They don't contain length data, nor anything else besides the address of their contents. Usually, you are wasting performance here by having to call strlen on a string whose length is theoretically known at compile time.

What are the alternatives? Since you seem to be using C++17, this is an ideal use case for std::string_view. If this is not the case, then the guidelines support library offer as variety of string spans. If both of these options are blocked for you, you could template your way out on the type of the parameter. As a last option, you could also just simply take a std::string const& or std::string&&.

About getNextPrefix

Overall, this method seems a bit unclear to me. Starting right from the definition, why do you have an anonymous first parameter? What is the point of it? This seems highly dubious to me. There are only few reasons to have such a parameter anywhere in the first place, and none of them seem to apply here. Does this method follow an interface requirement that you did not tell us? Seems unlikely since it is private.

Following, what kind of Frankenstein string is mes? When I was reading through the Format constructor, I was really confused at first because those last strings in the calls to this method didn't seem to make a lot of sense. In fact, these strings only make sense in the context of getNextPrefix.

It seems to me like you were hit by a sudden case of design pattern madness: Valuing the DRY principle above everything else, you reduced those two message strings to the bare minimum, divided every common divisor out. If this were normal code, that would be most likely fine, but we are talking about strings here.

Even if this method is private and no user ever would have to deal with this quirky interface, think about what you are actually doing when you are defining any method. You are abstracting away a certain functionality behind a name, and this name now has to convey its meaning where it replaces the code it contains. In your code, however, nothing conveys the fact (which is not at all obvious) that only part of a sentence should be passed to mes.

In general, splitting up what logically belongs to a single part of a string (such as a sentence, a phrase, a name, etc. depending on context) is almost never a good idea. You are certainly decreasing readability, at the gain of a few characters of saved code.

Another thing I am not a big fan of is using numerical indices instead of iterators. Doing so gains you very little, but decreases the semantic integrity of your code. After all, std::size_t is not a very useful name. All it says is "this here is an unsigned number", while std::string::iterator expresses "this here is a position in a std::string".

Iterators do have some drawbacks, such as the possibility of invalidation if the underlying container is moved, but those do not apply here, since everything you do that could be written using iterators is a single logical action.

This would also get rid of the (arguably) ugly std::string::npos, and the ugly find method of std::string (to be replaced by std::find).

One last point: std::function is quite heavy implementation wise. While it is, in general, not a design problem at all to use it (and I would go further and say that it is even a good practice), it can have pretty heavy stack and performance drawbacks. If you don't mind those, you can go on using it. If you do, you should make the type of test a template parameter, or go with the C way of passing a simple function pointer (which is arguably much worse style-wise, but better for code size).

Ignoring Values

std::ostream* ignore[] = {&printValue<I>(s)...};

is pretty ugly. In my opinion,

static_cast<void>(printValue<I>(s)...);

looks much nicer.

Includes

You are missing an include for std::size_t. Preferably, that would be #include <cstddef>, although there are at least five other headers guaranteed to define it. Similarly, you are missing #include <utility> for std::move, std::pair and the whole std::integer_sequence package, and #include <functional> for std::function. Finally, you are also missing #include <algorithm> for std::remove_if, #include <iterator> for std::begin and std::end, and #include <tuple> for everything std::tuple-related. Please be more careful around your includes!

Another thing that bothers me a little is that your include are not sorted alphabetically. While this is not a problem per se for such a small project as yours, it can grow to be a quite a nuisance in larger projects with dozens or more of includes. Ordering your includes from a to z makes getting an overview over them much easier and helps verify whether all includes are actually there.

Typos and Spelling Inconsistencies

Finally, the most pedantic and least useful code review point of all!

Format contains a member variable spelled formater. However, in the constructor, you have the comment

// Convert the next part of the string into a Formatter object.

Since "formatter" with two 't's is the correct spelling of the word, you might want to change you variable name.

Another comment of yours reads

// Take into account the Dynamic Width/Precesion prevent the format from being read.

I assume you meant "Precision" here.

In general, I would like to appeal to you to make more use of commas and punctuation in general throughout your comments. For example:

// For each argument we pass
// Pass a prefix string (might be empty) the formatter then the argument.

would be more correct (from a language point of view) if written as

// For each argument we pass:
// Pass a prefix string (might be empty), the formatter, then the argument.

I personally find it very important to not be sloppy with code (as I presume you do, since if you didn't, you would not be coming here for code review), which includes comments. The reason is that introducing sloppiness at one point tends to carry over to other parts of the work, at least for me. More importantly, however, comments are a way of communicating with other programmers; as such they should be clear and easy to understand, which is somewhat hindered by bad orthography and grammar.


This answer turned out to be kind of a massive wall of words. Most points I mentioned are opinion-based to a certain degree, so please take most of these as a 'Please consider...' instead of a 'Do!'. However, there is a single point which does have this imperative nature, the first point mentioned under Includes. If there is only a single thing you consider from this answer as a whole, please make it that point, since it severely endangers the validity of your program.

\$\endgroup\$
  • \$\begingroup\$ I don't really think you meant a 14-foot diameter laptop (perhaps 14-inch diagonal?), but I'm enjoying seeing that in my imagination. Polar coordinates? \$\endgroup\$ – Toby Speight Mar 5 '18 at 10:34
  • \$\begingroup\$ @TobySpeight Ah, obviously, my bad. What do you mean by polar cordinates? \$\endgroup\$ – Ben Steffan Mar 5 '18 at 15:04
  • \$\begingroup\$ Because a diameter is the distance across a circle (you probably meant diagonal - from corner to corner). \$\endgroup\$ – Toby Speight Mar 5 '18 at 15:13
  • \$\begingroup\$ @TobySpeight Oh, well, I don't seem to have been paying very much attention when I wrote that paragraph. Thank you for the hint! \$\endgroup\$ – Ben Steffan Mar 5 '18 at 15:21
  • \$\begingroup\$ Thanks for the input. Added set colorcolumn=100 to my .vimrc file. Trying to re-format that section to make it easier to read (and keep line lengths down). I'll give you a heads up when I push changes. \$\endgroup\$ – Martin York Mar 5 '18 at 16:51
1
\$\begingroup\$

Code

Ben Steffan notes missing includes, and various other issues, so I'll just add / reiterate the following:

  • doPrint doesn't compile with MSVC 2015 due to creating a zero sized array. It needs to be:

    std::ostream* ignore[] = { nullptr, &printValue<I>(s)... };
    
  • (Bug:) extra in getNextPrefix() is only initialized to zero, and not changed. I assume it's meant to track any %s removed from the prefix (so that %%% will work correctly - currently it doesn't).

  • getNextPrefix() is really confusing: The unnamed parameter. Passing in a function to "test" the next formatter here.


Design

The million dollar question: If this successfully validates printf arguments... why not just call printf?


Overall (and as Incomputable's answer to the previous review noted) printf formatting is not a good choice for C++. We know what the types to be printed are, so all the types specified in the format string are redundant. This also turns a compile time problem into a runtime error.

C#-style format strings are likely to be a better fit.


(Another point from Incomputable's previous answer): the design is currently part way between a function and a reusable class. Storing the arguments inside the class by reference is error-prone).

I'd suggest either:

  • A function (in which case the Format class should be unseen and inaccessible).
  • A reusable object that takes the format string in the constructor and parses it (as Format does now), and then allows the ostream operator (or an equivalent function) to be called with different arguments any number of times.

s << prefixString[I] << formater[I] << std::get<I>(arguments) forces one prefix string for every Formatter, and one Formatter for every argument value.

This 1:1:1 requirement is incorrect, due to the way * arguments for width and precision work. Removing this requirement would allow the following:

  • prefixStrings could be stored with std::string_view or iterators, instead of string copies.
  • No need for complicated logic tracking dynamic size when parsing in the Format / Formatter constructors.
  • No need for saveToStream.

This can be done by merging the formater and prefixstring containers into one storing a variant. We can then iterate the merged container (at runtime), alongside the arguments (statically).

Very roughly, something like the following:

#include "Variant.h" // use std::variant instead...

#include <cassert>
#include <vector>
#include <string>

struct StringPrimitive
{
    std::string::const_iterator start, end; // use std::string_view!
};

struct FormatSpecifier
{
    // ... TODO: take actual width / precision
    bool NeedsWidth() const { return !m_widthSet; }
    bool NeedsPrecision() const { return !m_precisionSet; }

    void SetWidth(int) { m_widthSet = true; }
    void SetPrecision(int) { m_precisionSet = true; }

    bool m_widthSet = false;
    bool m_precisionSet = false;

    // ... TODO: everything else!
};

// nicked from a stackoverflow post:
template <size_t N, typename... Args>
decltype(auto) magic_get(Args&&... as) noexcept {
    return std::get<N>(std::forward_as_tuple(std::forward<Args>(as)...));
}

struct Formatter
{
    std::string m_formatString;

    using PrimitiveT = Variant<StringPrimitive, FormatSpecifier>; // use std::variant instead...
    using PrimitivesT = std::vector<PrimitiveT>;
    PrimitivesT m_primitives;

    explicit Formatter(std::string const& formatString):
        m_formatString(formatString)
    {
        // ... TODO: parse the format string properly!

        m_primitives.push_back(PrimitiveT(StringPrimitive{ m_formatString.begin(), m_formatString.begin() + 13u }));
        m_primitives.push_back(PrimitiveT(FormatSpecifier()));
    }

    template<std::size_t Index, class... Args>
    void Print(std::true_type, std::ostream& os, PrimitivesT::iterator p, Args&&... args) // no more args to process
    {
        if (p == m_primitives.end()) // done! end the "recursion"
            return;

        if (p->IsType<StringPrimitive>())
        {
            auto& string = p->Get<StringPrimitive>();

            os << std::string(string.start, string.end); // ... TODO: print string held in StringPrimitive

            ++p; // consume primitive

            Print<Index>(std::true_type(), os, p, args...);
        }
        else if (p->IsType<FormatSpecifier>())
        {
            throw std::runtime_error("extra format specifier in string, or missing argument(s)!");
        }
        else
        {
            assert(false); // uhh...
        }
    }

    template<std::size_t Index, class... Args>
    void Print(std::false_type, std::ostream& os, PrimitivesT::iterator p, Args&&... args)
    {
        if (p == m_primitives.end())
            throw std::runtime_error("missing format specifier in string, or extra argument(s)!");

        if (p->IsType<StringPrimitive>()) // or some kind of visit function...?
        {
            auto& string = p->Get<StringPrimitive>();

            os << std::string(string.start, string.end); // ... TODO: print string held in StringPrimitive

            ++p; // consume primitive

            Print<Index>(std::false_type(), os, p, args...);
        }
        else if (p->IsType<FormatSpecifier>())
        {
            auto& formatSpecifier = p->Get<FormatSpecifier>();

            if (formatSpecifier.NeedsWidth())
            {
                formatSpecifier.SetWidth(magic_get<Index>(args...));
            }
            else if (formatSpecifier.NeedsPrecision())
            {
                formatSpecifier.SetPrecision(magic_get<Index>(args...));
            }
            else
            {
                os << magic_get<Index>(args...); // ... TODO: print value at index using the format specifier

                ++p; // consume primitive
            }

            Print<Index + 1>(std::integral_constant<bool, (sizeof...(Args) == Index + 1)>(), os, p, args...);
        }
        else
        {
            assert(false); // uhh...
        }
    }

    template<class... Args>
    std::ostream& operator()(std::ostream& os, Args&&... args)
    {
        Print<0>(std::integral_constant<bool, (sizeof...(Args) == 0)>(), os, m_primitives.begin(), args...);

        // ... TODO: clear all the dynamic widths / precisions in the format specifiers

        return os;
    }
};

#include <iostream>

int main()
{
    auto f = Formatter("test string %%%*.*d\n");

    f(std::cout, 5, 5, 12);

    std::cout << std::endl;

    f(std::cout, 1278); // 1 arg because we didn't clear the dynamic widths / precisions... this should throw

    std::cout << std::endl;
}
\$\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.