3
\$\begingroup\$

Motivation / Task

An apparently trivial problem....

A CLI app tries to make use of box drawing characters. But depending on the stdout device this may not be appropriate so there is a -t option to use pure ASCII text.

The concept is just an example, this applies anywhere where we want to select from two or more sets of static constants at runtime. Those constants should be accessed via compile time checkable "labels" to avoid mistakes in the config. Ideally the solution should be simple/clean and produce very little runtime code (just a single pointer?).

Desired usage Syntax

This is indicative only, but accessing the constants should be terse:

int main(int argc, char* argv[]) {

  // multiple possibilities here, include inheritance
  auto t = Terminal(std::strcmp(argv[1], "-t"))

  // here we want the syntax to be short
  std::cout << "text" << t.m.horiz << t.m.right_t << '\n';

}

Implementation

After some discussion of using inheritance, it proved difficult to get any benefit out of a class hierarchy (the member variables/constant don't really inherit) and ended up with much boilerplate and complication that didn't add anything.

The proposed solution uses aggregate initialization with designated initializers (well supported C99ext / C++20).

Note that we could safely override only some of the constants in the default mode when defining the extended mode if desired. All the "keys" are automatically checked at compile time, which prevents mistakes during maintenance (potentially hundreds of constants per "mode").

namespace vt100 {

struct Mode {
  const char esc[2]          = {};
  const char cls[5]          = {};
  const char bd_on[4]        = {};
  const char bd_off[4]       = {};
  const char underline_on[5] = {};

  const char black[6]        = {};
  const char red[6]          = {};
  const char green[6]        = {};
  const char yellow[6]       = {};
  const char blue[6]         = {};
  const char magenta[6]      = {};
  const char cyan[6]         = {};
  const char white[6]        = {};

  const char reset[5]        = {};

  const char horiz           = '-';
  const char vert            = '|';
  const char right_t         = '|';
  const char left_t          = '|';
  const char bottom_t        = '|';
  const char top_t           = '|';
  const char intersec        = '|';
};

static constexpr Mode text_mode{};

// warning: this is a C99 extension until C++20 comes in
// but it nicely compile checks and self documents the code
// supported by gcc4.7, clang3.0 and msvc19.21
static constexpr Mode box_draw_mode{
    .esc          = "\x1b",
    .cls          = "\x1b[2J",
    .bd_on        = "\x1b(0",
    .bd_off       = "\x1b(B",
    .underline_on = "\x1b[4m",

    .black        = "\x1b[30m",
    .red          = "\x1b[31m",
    .green        = "\x1b[32m",
    .yellow       = "\x1b[33m",
    .blue         = "\x1b[34m",
    .magenta      = "\x1b[35m",
    .cyan         = "\x1b[36m",
    .white        = "\x1b[37m",

    .reset        = "\x1b[0m",

    .horiz        = '\x71',
    .vert         = '\x78',
    .right_t      = '\x75',
    .left_t       = '\x74',
    .bottom_t     = '\x76',
    .top_t        = '\x77',
    .intersec     = '\x6e',
};

struct Terminal {
  const Mode& m;
  constexpr Terminal(bool textmode = true) : m{textmode ? text_mode : box_draw_mode} {}
};
} // namespace vt100


int main(int argc, char* argv[]) {

  auto t1 = vt100::Terminal{true};
  auto t2 = vt100::Terminal{false};

  // usage
  // std::cout << t1.m.cls << '\n' << t1.m.right_t << '\n';
  // std::cout << t2.m.cls << '\n' << t2.m.right_t << '\n';

}

Here is a godbolt. Note that clang>3 and gcc>4.7 compile the designated initialisers just fine already.

On -O1 we can clearly see the strings laid out in .text segment and literally the only thing that happens during Terminal construction is the setting of a single pointer for Mode& m to point to one of the 2 structs.

  mov qword ptr [rdi], rcx
  • Did I miss an obvious way to do this differently and cleanly?
  • Perhaps a simple way to use inheritance or some other way altogether?
  • Did I get the use of const and constexpr right?
\$\endgroup\$
6
  • 1
    \$\begingroup\$ You should not change the question after you have received an answer, since this could invalidate the answers. For code review, once you have updated your code, just create a new code review question. Also, not only you, but also others can learn from your mistakes. \$\endgroup\$ – G. Sliepen Dec 21 '19 at 21:54
  • \$\begingroup\$ You're right. However, the change was to revert to what I had originally when I posted it. const char* I felt the micro optimisation of char[] was just a distraction (added as an EDIT later) and missed the main point for people reading it later. So I removed it again. Also your answer (apart from one of the comments below) does not adress the char * point. Your answers focuses on the main issues, which is good. So I was trying to make it more relevant for other readers. Not sure if that's wrong? \$\endgroup\$ – Oliver Schönrock Dec 21 '19 at 22:04
  • \$\begingroup\$ Ah, I didn't know it was a revert. Indeed I don't mention the char issue in the answer itself, but it is discussed in the comments. It's not a big issue here, just something to keep in mind :) \$\endgroup\$ – G. Sliepen Dec 21 '19 at 22:19
  • \$\begingroup\$ Yup! for sure. Should probably have resisted editing it to put the char[] stuff in, in the first place ! \$\endgroup\$ – Oliver Schönrock Dec 21 '19 at 22:33
  • \$\begingroup\$ Please don't touch your code after answers start coming in. Unlike a forum, this place does not expect you or wishes you to keep your code up-to-date. All answers should be reviewing the same revision of the code, we've seen incredible messes created in the past if we don't enforce this. I've rolled back to the revision which was actual at the time the answer got posted. \$\endgroup\$ – Mast Dec 22 '19 at 13:28
3
\$\begingroup\$

Using class inheritance

You can use class inheritance, the constants do inherit. I don't think the boilerplate is that much compared to the constexpr variable approach with designated initializer. It would look like:

struct Mode {
    const char esc[2] = {};
    ...
};

struct TextMode: Mode {
};

struct BoxDrawMode: Mode {
    BoxDrawMode(): Mode {
        .esc ="\x1b",
        .cls ="\x1b[2J",
        ...
    } {
    }
};

So just a few extra lines to declare the constructor. However, I don't see any benefit over the approach you posted, and one big issue is that BoxDrawMode has a non-trivial constructor, so you can't instantiate a constexpr variable of that type.

const and constexpr

I think you got that right. For performance it will not matter much whether text_mode and box_draw_mode are static const or static constexpr though.

Consider making Terminal templated

One issue with your approach is that the types of the member constants in Mode are fixed. Either you are wasting space by having arrays for things like colors that are not used by text_mode, or you might actually not have big enough arrays to hold data for future modes. For example, if you would add a Unicode box drawing mode, then you would need more than a single char for horiz, vert and so on.

However, if you make struct Terminal a template that accepts any type of struct that contains the mode information, then you don't have this issue. For example:

struct TextMode {
    const char esc[] = "";
    ...;
    const char intersec = '|';
};

struct BoxDrawMode {
    const char esc[] = "\x1b",
    ...
    .intersec = '\x6e';
};

template<typename Mode = TextMode>
struct Terminal {
    constexpr Terminal() {...};
    ...
};

And then use it like so:

vt100::Terminal<TextMode> t1;
vt100::Terminal<BoxDrawMode> t2;
vt100::Terminal t3; // selects TextMode by default

The drawback is that if you have to pass an instance of Terminal as an argument to another function, then that function has to handle all possible variants of Terminal, so it likely has to be templated itself, or you have to derive Terminal<> from a base class.

With this approach, you can also change the member variables of the various modes from being const to static constexpr.

\$\endgroup\$
11
  • \$\begingroup\$ Thank you. That's helpful thought provoking feedback. I am still thinking about your inheritance suggestion and about the templated suggestion. One question on the latter though. How could the application switch between vt100::Terminal<TextMode> t1 and vt100::Terminal<BoxDrawMode> t2 at runtime? They a different type so we can't write: t = condition ? t1 : t2; // then use t... \$\endgroup\$ – Oliver Schönrock Dec 21 '19 at 19:44
  • \$\begingroup\$ I agree that the empty vars in text_mode are wasteful and that we cannot change them in one terminal without changing the other. However when the main app uses the terminal it wants to not be concerned with what terminal it has, and just request t.m.green and for that to then happen to best of that terminal's ability (which may be not at all). So the "hard types" in the mode variables give us the safety of writing consistent configs and they ensure that the different terminals can be used iterchangably. They are "sort of" the API. Which is why I initially thought of inheritance. \$\endgroup\$ – Oliver Schönrock Dec 21 '19 at 19:49
  • \$\begingroup\$ If you want the templated approach to be usable with runtime selection, you will have to make it derive from a base class, and then do something like: std::unique_ptr<vt100::TerminalBase> t = textmode ? std::make_unique<vt100::Terminal<TextMode>> : std::make_unique<vt100::Terminal<BoxDrawMode>>;. Of course, that's probably inefficient by itself. \$\endgroup\$ – G. Sliepen Dec 21 '19 at 20:19
  • \$\begingroup\$ To handle arbitrary length strings, I would convert all the members to be const char *. Although it seems that you are adding pointers and indirections to the mix, you are really not; the compiler will optimize them away. \$\endgroup\$ – G. Sliepen Dec 21 '19 at 20:21
  • 1
    \$\begingroup\$ You need to have a pointer when using polymorphism, and std::unique_ptr avoids having to manually write new and delete. The template buys you the freedom to change the type of the member constants of the modes. \$\endgroup\$ – G. Sliepen Dec 21 '19 at 20:36

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.