2
\$\begingroup\$

I'm working on a benchmarking library. The code below is a textual serializer of a complex benchmark results structure (there will be more fields). Main feature is building up the desired output format using library defined operands.

From end user point of view, it will look like this:

// ... benchmarking code

auto results = myBenchmark.getResults();

// Some output options for time related operands.
TimeOperandOptions<microseconds_d> o{};
o.Precision = 2;

TextPrinter printer{};

// UserLabel, Min are operands.
printer.format() << TextFormat::UserLabel() << " - MinRT: " << TextFormat::Min(o) 
    << " MaxRT: " << TextFormat::Max(o) << "\t" << "Average: " << TextFormat::Average(o);

std::cout << printer(results);

The main structure is the following:

  • TextPrinter is a base class with call operator (returns final formatted string).
  • TextFormat holds and manages operands, call them in a sequence they were added and builds up return string. In the same file I put static functions returning operands.
  • Operands derives from OperandBase. process() returns formatted field fro specific result piece. TimeOperands derive from TimeOperand for duration formatting utilities.

The entire library is going to be larger, but that's the high level design I'm planning to use. Below is demo presenting working code for Min operand, this source code is best to read downside-up.

#include <cassert>
#include <chrono>
#include <string>
#include <list>
#include <iomanip>
#include <sstream>
#include <iostream>


// Helpers
using BaseUnitType = std::chrono::nanoseconds; // Time unit used in measurements.

struct Results {
    BaseUnitType Min{};
};


template <typename T = BaseUnitType>
struct TimeOperandOptions {
    int  Precision{2};
    // More options here
};

// OPERAND BASE
struct OperandBase {
    virtual std::string process(const Results& resultsToPrint) const = 0;
    virtual ~OperandBase() {};
};

// TIME OPERAND BASE CLASS
template <typename T>
struct TimeOperand : public OperandBase {
    protected:
        TimeOperandOptions<T> options{};

        std::string timeFormat(const BaseUnitType& time) const {
            T tempTime = std::chrono::duration_cast<T>(time);
            std::string buffer{};

            if (std::is_floating_point<double>::value) {              
                std::stringstream floatStream;
                floatStream << std::fixed << std::setprecision(options.Precision) << tempTime.count();
                buffer = floatStream.str();
            } else {
                buffer = std::to_string(tempTime.count());
            }

            // More formatting...

            return buffer;
        }

    public:
        TimeOperand(TimeOperandOptions<T> options) :    
            options{options}
        {};

        virtual std::string process(const Results& resultsToPrint) const = 0;
};

// OPERANDS

// Min inherits from TimeOperand for more time formatting utils.
// Other may inherit from OperandBase (like string operand)
template <typename T>
struct MinOperand : public TimeOperand<T> {
    MinOperand(TimeOperandOptions<T> options = {}) :
        TimeOperand<T>{options}
    {};

    std::string process(const Results& resultsToPrint) const override {
        return TimeOperand<T>::timeFormat(resultsToPrint.Min);
    }
};

// FORMAT
// Stores list of operands passed by operator<<. 
class TextFormat {
private:
    std::list<std::unique_ptr<OperandBase>> _formatOperands{};

public:
    TextFormat() {};

    // .. Copy, move - deleted

    TextFormat& operator<<(OperandBase* rhs) {
        _formatOperands.emplace_back(rhs);
        return *this;
    };

    // Calling process all operators and builds up return string.
    std::string operator() (const Results& resultsToPrint) const {
        std::ostringstream output;

        for (const auto& op : _formatOperands) {
            output << op->process(resultsToPrint);
        }

        return output.str();
    }

    template <typename T>
    static OperandBase* Min(TimeOperandOptions<T> options);
};

// Static returning Min operand pointer.
template <typename T>
OperandBase* TextFormat::Min(TimeOperandOptions<T> options) {
    return new MinOperand<T>{options};
}

// PRINTER
class TextPrinter {
private:
    TextFormat _format{};

public:
    TextFormat& format() {
        return _format;
    }

    std::string operator() (const Results& resultsToPrint) const {
        return _format(resultsToPrint);
    }
};

// PROGRAM
template <class Rep>
using microseconds_t = std::chrono::duration<Rep, std::micro>;
using microseconds_d = microseconds_t<double>;

int main() {
    Results results{}; // Returned from benchmarking function.
    results.Min = BaseUnitType{500}; // Minimal recorded time
    TextPrinter printer{};

    TimeOperandOptions<microseconds_d> customOptions{};
    customOptions.Precision = 2;

    printer.format() << TextFormat::Min(customOptions);

    std::cout << printer(results);
}

I'd really love to get some honest feedback, especially on entire design and structure. I want this formating builder to be extensible and easy to use, performance has a lower priority but any comment will be appreciated. What can be improved, how would you organize this code, better namings, any bugs, thoughts, comments or improvements.

\$\endgroup\$
  • 1
    \$\begingroup\$ add following warning to compilation -Werror=shadow \$\endgroup\$ – Artemy Vysotsky Sep 26 '17 at 4:16
  • \$\begingroup\$ @ArtemyVysotsky Thank you very much, this warning triggers in multiple places in my code. I wasn't aware that defining parameter name equal to member name is wrong, can this cause any real complication, or is just a good style to use different names? \$\endgroup\$ – Outshined Sep 26 '17 at 9:00
  • \$\begingroup\$ Only that you can use one while thinking you are actually using another. It will be especially dangerous if your order of initialization is not the same as declarations and you'll be using yet not initialized member instead of parameter. I prefer to have slightly different names to be sure what I'm using \$\endgroup\$ – Artemy Vysotsky Sep 26 '17 at 9:10
  • \$\begingroup\$ @ArtemyVysotsky Thats reasonable explanation, thanks for advice. \$\endgroup\$ – Outshined Sep 26 '17 at 9:39
1
\$\begingroup\$

After trying your sample I have only 2 notes -

Use namespace for your library.

And provide microseconds_d, milliseconds_d, etc as part of the library as well (under the same namespace to avoid possible conflicts) so user can easily use ready-made types to switch from one units to another without thinking and plying with typedefs in "PROGRAM" code.

For extending - I feel like have possibility to configure printer to add units literal would be nice and also switch between number output 12345678.50ms and time output 00:01:23.234

EDIT

To make clean compilation

  • need to fix warning shown by gcc when compiled with -Werror=shadow
  • need to include <memory> to avoid error C2039: 'unique_ptr': is not a member of 'std' in VS2017

I strongly feel that any code to be distributed as library should be compiled at highest warning level since it is impossible to perdict the compilation settings of the customer's project

\$\endgroup\$
  • \$\begingroup\$ It would be great if you would add the note from comments about enabling warning about shadowing. It seems like the edit about it was not made, so it would be still valid review point. \$\endgroup\$ – Incomputable Sep 27 '17 at 7:23

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.