0
\$\begingroup\$

I have this code that writes the initial configuration of a simulator I'm writing to stdout. args is an instance of type struct Arguments, and all the members of said struct are used in the singular function below. I was hoping for some hints to simplify the code and get the alignment correct.

I have considered using <cstdio> and simply writing the output I need to a buffer string with sprintf (this code is guaranteed to be shorter) and then writing to the console with either std::cout or printf. However, I have tried my best to avoid using any C libraries in this project.

struct Arguments {
    std::string_view protocol{"MESI"};
    std::filesystem::path benchmark{"data/bodytrack"};
    uint32_t cacheSize{4096};
    uint8_t associativity{2};
    uint16_t blockSize{32};
    uint32_t numBlocks{4096 / 32};

    Arguments() = default;
    explicit Arguments(const char* argv[]);
  };

void Runner::printConfig() const {
  std::stringstream ss;
  ss << "====================================" << std::endl
     << "Cache protocol:\t\t" << args.protocol << std::endl
     << "Cache size:\t\t" << args.cacheSize << " B";

  if (args.cacheSize > (1 << 10) && args.cacheSize < (1 << 20))
    ss << " (" << std::setprecision(4) << static_cast<double>(args.cacheSize) / static_cast<double>(1 << 10) << " KiB)"
       << std::endl;
  else if (args.cacheSize > (1 << 20))
    ss << " (" << std::setprecision(4) << static_cast<double>(args.cacheSize) / static_cast<double>(1 << 20) << " MiB)"
       << std::endl;
  else ss << std::endl;

  ss << "Block size:\t\t" << args.blockSize << " B" << std::endl;

  ss << "Associativity:\t\t";
  if (args.associativity == 1) ss << "direct-mapped" << std::endl;
  else if (args.associativity == args.numBlocks) ss << "fully associative" << std::endl;
  else {
    ss << std::to_string(args.associativity) << "-way set-associative" << std::endl
       << "Cache blocks:\t\t" << args.numBlocks << " (" << (args.numBlocks / args.associativity) << " per set)"
       << std::endl
       << "====================================";
    std::cout << ss.view() << std::endl;
    return;
  }
  ss << "Cache blocks:\t\t" << args.numBlocks << std::endl << "====================================";

  std::cout << ss.view() << std::endl;
}

Run with the arguments Dragon bodytrack 1024 2 16, this outputs:

====================================
Cache protocol:     Dragon
Cache size:     1024 B (1 KiB)
Block size:     16 B
Associativity:      2-way set-associative
Cache blocks:       4 (2 per set)
====================================

N.B.

  1. I'm aware that the separators are not long enough: that's also in the pipeline).
  2. I wish I could use <format>, but this project is cross-platform and only MSVC has implemented it at all.
  3. Ideally no large third-party libraries (Boost, etc).
\$\endgroup\$
4
  • \$\begingroup\$ You're missing the definition of args. Without that, it's very hard to review this code. \$\endgroup\$ Nov 17 at 12:21
  • 1
    \$\begingroup\$ Have a look at fmt - github.com/fmtlib/fmt \$\endgroup\$
    – jdt
    Nov 17 at 13:14
  • \$\begingroup\$ Welcome to Code Review@SE. Please heed How do I ask a Good Question?, especially in picking a title for your question. \$\endgroup\$
    – greybeard
    Nov 17 at 17:13
  • \$\begingroup\$ There are several C++ formatting libraries out there. Boost Format I even wrote one ThorIOUtil \$\endgroup\$ Nov 17 at 22:57
4
\$\begingroup\$
  • Don't use std::endl unless you actually need flushing behavior. For a newline, just output "\n".

  • Use the manipulators from <iomanip> to get fixed column sizes. Specifically std::setw(n) to set the output width to size n, and std::left or std::right align things if necessary.

  • Consider sticking to one line of output per ss << ... chain (except where it's necessary to break a line up).

  • As mentioned by upkajdt in the comments, you could use fmtlib where std::format isn't implemented yet. (The two should be functionally equivalent, so you could use a wrapper function to forward calls to the appropriate backend).


  if (args.cacheSize > (1 << 10) && args.cacheSize < (1 << 20))
    ss << " (" << std::setprecision(4) << static_cast<double>(args.cacheSize) / static_cast<double>(1 << 10) << " KiB)"
       << std::endl;
  else if (args.cacheSize > (1 << 20))
    ss << " (" << std::setprecision(4) << static_cast<double>(args.cacheSize) / static_cast<double>(1 << 20) << " MiB)"
       << std::endl;
  else ss << std::endl;

We can simplify this slightly by checking for the largest size first, and moving the newline out of the condition:

if (args.cacheSize > (1 << 20))
    ss << " (" << std::setprecision(4) << static_cast<double>(args.cacheSize) / static_cast<double>(1 << 20) << " MiB)";
else if (args.cacheSize > (1 << 10))
    ss << " (" << std::setprecision(4) << static_cast<double>(args.cacheSize) / static_cast<double>(1 << 10) << " KiB)";

ss << "\n";

Converting to KiB / MiB could perhaps be done in a separate function.


  ss << "Associativity:\t\t";
  if (args.associativity == 1) ss << "direct-mapped" << std::endl;
  else if (args.associativity == args.numBlocks) ss << "fully associative" << std::endl;
  else {
    ss << std::to_string(args.associativity) << "-way set-associative" << std::endl
       << "Cache blocks:\t\t" << args.numBlocks << " (" << (args.numBlocks / args.associativity) << " per set)"
       << std::endl
       << "====================================";
    std::cout << ss.view() << std::endl;
    return;
  }
  ss << "Cache blocks:\t\t" << args.numBlocks << std::endl << "====================================";

  std::cout << ss.view() << std::endl;

Is it really necessary to conditionally output the blocks?

ss << std::setw(20) << "Associativity:";

if (args.associativity == 1) ss << "direct-mapped";
else if (args.associativity == args.numBlocks) ss << "fully associative";
else ss << std::to_string(args.associativity) << "-way set-associative";

ss << "\n";

ss << std::setw(20) << "Cache blocks:" << args.numBlocks << " (" << (args.numBlocks / args.associativity) << " per set)" << "\n";
ss << "====================================";

std::cout << ss.view() << std::endl;
\$\endgroup\$
1
\$\begingroup\$

Use a helper function:

void print_KorM (std::ostream& o, double val)
{
    o.precision(4);
    constexpr double K = 1.0 << 10;
    constexpr double M = 1.0 << 20;
    if (val > M)  o << (val/M) << "MiB";
    else if (val > K)  o << (val/K) << "KiB";
    else o << val;
}

Note that there is no casting needed in the implementation or by the caller.

In general, break out all the complicated format it this way or that way depending on the range or other values into their own functions, even if they are only used once. Then your principal function is just outputting captions and a simple statement for each value. (e.g. numBlocks)


You write: ss << std::to_string(args.associativity)
This is inefficient as you create a string object, rather than formatting directly into the stream. I'm guessing you do this because writing ss << args.associativity directly formats it as a character rather than an integer? The simple, zero-overhead fix for that is to write: ss << 0+args.associativity.

You could write int(args.associativity) but this begs the question of what is the proper type to use actually. By using 0+ it will invoke the promotion rules to give a type that we know is at least an int (since the literal 0 is of type int) thus not a char-sized type that causes issues. It will also continue to work unchanged if you ever change the definition of associativity to a different type, such as uint32_t, as the arithmetic promotion rules will cover that.

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