Observation

This is a common one beginners do. And to be blunt I wish they would not do this. It would be much better to learn how to sue the system logging tools.

Questions

• Are there any apparent issues with how I implemented the macro?

I don't see any. But the way you have done it there are no advantages of using the macro over a normal inline function. Also the normal inline function is probably safer and better because of the extra type checking.

• Can there be any improvements made to it?

Macros if used differently could be good. You can turn them off at compile time and save the cost of evaluating any parameters. Though with modern C++ lambda's you can potentially have the same affect.

If you want to do this you should log to the syslog rather than your own personal logging system. Now saying that there is nothing wrong with wrapping syslog with your own code is not a bad idea.

• Is the intent clear on its usage?

Sure. I don't see anything particularly wrong. But it does require that you build the message before hand (there is no way to build the message as part of the message statement (OK you can do some simple stuff, but anything complex would break the macro (i.e. anything with a comma)).

• I'm also interested in any other feedback.

Sure one second

Code Review

What are you using msg_ for?

class FileWriter {
    std::string filename_;
    std::ostringstream msg_;
public:

You don't use it in any methods.

You are forcing people to build a string stream them logging the string you can extract from the stream.

    void operator()(std::ostringstream& msg) {
        std::ofstream out("log.txt", std::ios::app);
        out << msg.str();        
    }

Why not just allow people to pass a std::string. They you can pass a simple string without having to build a string stream first. Even better would be allow you to chain a series of objects with the operator<<.

How about this:

class LokiFileWriter;
class LokiFileWriterStream
{
    std::ofstream   file;
    friend class LokiFileWriter;

    // Private so only LokiFileWriter can create one.
    LokiFileWriterStream(std::ofstream&& output)
        : file(std::move(output))
    {}
    public:
        LokiFileWriterStream(LokiFileWriterStream&& move) = default;
        template<typename T>
        LokiFileWriterStream& operator<<(T const& item)
        {
            // Send the T to the file stream.
            // Then return a reference to allow chaining
            file << item;
            return *this;
        }
        ~LokiFileWriterStream()
        {
            // When the expression is closed
            // We will close the file stream.
            //
            // But remember that the move constructor is available
            // So objects that have been moved move the stream object
            // an object that has been moved from has a file object that
            // is no longer valid (calling close() will fail in some way)
            // but it is a valid object that we are allowed to call close on
            file.close();
        }
};
class LokiFileWriter
{
    std::string filename;
    public:
    LokiFileWriter(std::string const& filename)
        : filename(filename)
    {}
    template<typename T>
    LokiFileWriterStream operator<<(T const& item)
    {
        // We create a stream object.
        LokiFileWriterStream stream(std::ofstream(filename, std::ios::app));
        stream << item;

        // The stream object is returned forcing a move
        // of the object to external calling frame.
        // This means the object inside this function may be
        // destroyed but the file object it contains has already been
        // moved and thus not destroyed.
        return stream;
    }
};

int main()
{
    LokiFileWriter  out("MyLogFile");
    // The first << creates the `LokiFileWriterStream`
    //    Each subsequent << returns a reference to the same object.
    out << "Test" << 123 << " Plop";
    //    At the end of the expression `LokiFileWriterStream` goes
    //    out of scope and we destroy the object which calls the
    //    destructor which then calls the close method.
}

---

Sure. This is useful.

static std::map<MsgTy, std::string> msg_id{
    {MsgTy::OK, {"OK: "}},
    {MsgTy::WARNING, {"WARNING: "}},  
    {MsgTy::ERROR, {"ERROR: "}},
    {MsgTy::CRITICAL, {"CRITICAL: "}}
};

But I would put it inside a method to make using it simple:

std::string const& to_string(MsgTy const& msg)
{
    static std::map<MsgTy, std::string> msg_id{
        {MsgTy::OK, {"OK: "}},
        {MsgTy::WARNING, {"WARNING: "}},  
        {MsgTy::ERROR, {"ERROR: "}},
        {MsgTy::CRITICAL, {"CRITICAL: "}}
    };
    return msg_id[msg];
 }

You may think this is a bit trivial. But think of the situation where your enum is passed to a function that is has a template parameter and it would normally use to_string() to convert to a string.

 template<typename t>
 void print(T const& object)
 {
      using std::to_string;
      std::cout << to_string(object);   // This would work for
                                        // your enum just like all
                                        // other types that support
                                        // to_string in the standard.
 }

---

I think you have over complicated this:

#define messaging(MsgTy, msg, log2file) do { 
    std::ostringstream strm; 
    ... OK
    ... WARNING
    ... ERROR
    ... CRITICAL
    }

I would create a separate macro for each type of message:

    #define messagingOK(msg, log2file)                 
    do {                                               
        std::ostringstream strm;                       
        strm << to_string(MsgTy::OK) << (msg) << "n"; 
        std::cout << strm.str();                       
        if((log2file) == true) {                       
            FileWriter fw("log.txt", strm);            
        }                                              
    } while(0)

That way I can turn on/off each macro at compile time. I probably don't want to log OK items in the production version so I would want to turn that off.

It is no moral difficult to use this than your version.

    messagingOK("Hi", true);
    messaging(OK, "Hi", true);

---

Now the reason to use macros is that you can turn them off and the cost of using the macros is reduced to zero!

If you had written this as an inline function it would look like this:

template<typename... Args>
inline void messagingOK(bool log2File, Args... const& args)
{
#if TURNON_OK
/* STUFF HERE */
#endif
}

The trouble here is that all the args are required to be evaluated (even if the function is inlined and the parameters are not used. The language guarantees that all the parameters are fully evaluated.

This is why we use macros like this:

#if TURNON_OK
#define messagingOK(msg, log2file)     /* STUFF HERE */
#else
#define messagingOK(msg, log2file)
#endif

So when you turn the macro off the cost of building the parameters is reduced to zero in this situation as they don't exist.

---

OK. So you got the correct reason for using the macro but your function does not allow you to use the macro in a way that makes this possible.

 // notice the brackets around the msg.
 // This means the expression inside the macros must be expanded first
 // unfortunately that does not work for the above
 strm << msg_id[(MsgTy)] << (msg) << 'n';

 // This will fail as the message part
 // will be included inside the brackets and thus must
 // be evaluated first with the stream object so you get
 // a compiler failure.
 messaging(OK, "OK: " << 15 << " Testing", true);

So you could move this into a function and pass the parameters to that and convert to a string.

 // Unfortunatel this also fails.
 // This time because of the way the macros interacts with commas.
 messaging(OK, buildString("OK: ", 15, " Testing"), true);

So now you have to build the string external to the macro:

 std::string message = std::string("OK: ") + 15 + " Testing";
 messaging(OK, message, true);

Now if I turn off the macro messaging we are still evaluating the string message so there is no advantage to using the macro.

---

If we go back to functions we can put off evaluation of parameters by using lambdas.

 inline void message(std::function<void(std::ostream)>&& messagePrinter)
 {
 #if TURNON_OK
     messagePrinter(std::cerr);
 #endif
 }

Here we are passing a function object. Creating a function object is usually very cheap so creating this object should be cheap and the cost is only invoked when the function is invoked.

 // The cost of the function `add()` is only payed
 // if we actually want generate the error message.
 message([](std::ostream& out){
     out << "This " << add(12, 3) << " a " << test << "n";
 });

---

Sure you want to throw a string?

throw strm.str();

This throws a std::string. This is not derived from std::exception. So youe code does not get caught in this catch...

    catch (const std::exception& e) {
        std::cerr << e.what() << std::endl;
        return EXIT_FAILURE;
    }

You’ve already noted that this could be done better without macros, so I won’t belabour the point. I will note, though, that your goal—“to refresh [your] skills at writing good solid macros”—makes about as much sense as refreshing your skills at writing code on punch cards. You are exercising an archaic practice that is dying out, and is unwelcome in any modern project.

enum MsgTy {
    OK = 0,
    WARNING,
    ERROR,
    CRITICAL,
};

In modern C++, you should use a strong enum—an enum class. That way your enumerators won’t pollute the namespace.

Speaking of polluting the namespace, the almost universal convention in C++ is that all-caps identifiers are used for preprocessor defines. By using them in this case, you run the risk of someone else’s macro definitions fuggering up your enum. And given that having a macro named something like ERROR is highly likely in large enough projects, you’re really cruising for a bruising here. (Actually, POSIX reserves everything starting with E followed by a digit or uppercase letter… so you’re REALLY asking for trouble with that in particular.)

I’m also not keen on the name MsgTy. Seems a little ugly and obtuse. I get that you want it to be short but… this seems a bit much.

class FileWriter {
    std::string filename_;
    std::ostringstream msg_;
public:
    FileWriter(const std::string& filename, std::ostringstream& msg) 
      : filename_{ filename } {
        operator()(msg);
    }

    void operator()(std::ostringstream& msg) {
        std::ofstream out("log.txt", std::ios::app);
        out << msg.str();        
    }
};

Oi, this class is….

First off… what is the point of the data members? You don’t use either of them.

Second… what’s the point of the function call operator? You could just as well do all the work in the constructor. You never use the function call operator anywhere else.

Third… what’s the point of taking the argument as a string stream when you just go ahead and re-format it through a file stream? You’re double-formatting it.

This entire class could boil down to:

struct FileWriter
{
    FileWriter(std::string_view filename, std::string_view msg)
    {
        auto out = std::ofstream{filename, std::ios_base::app};
        out << msg;
    }
};

But even then, I’m not sure this is a great idea, because you’re reopening the file every time you’re writing a new log line, then closing it after. That doesn’t seem like a great idea, efficiency-wise.

A better idea would be to open the file once, and keep it open. Then just syncronize your writes (assuming you care about concurrency, which it sure doesn’t look like it), and flush after every log line. Normally std::endl is a terrible idea… but flushing after every line is exactly the singular use case it’s actually intended for.

static std::map<MsgTy, std::string> msg_id{
    {MsgTy::OK, {"OK: "}},
    {MsgTy::WARNING, {"WARNING: "}},  
    {MsgTy::ERROR, {"ERROR: "}},
    {MsgTy::CRITICAL, {"CRITICAL: "}}
};

As far as mapping enumerators to strings, this isn’t really the best way to go about it. It’s astoundingly inefficient and clunky for what should be a trivial task. A std::map is a heavyweight object… using it for literally 4 elements is… not a good usage.

A better solution is to either implement a to_string() function:

constexpr auto to_string(MsgTy mt)
{
    using namespace std::string_view_literals;

    switch (mt)
    {
    case MsgTy::OK:
        return "OK"sv;
    case MsgTy::WARNING:
        return "WARNING"sv;
    case MsgTy::ERROR:
        return "ERROR"sv;
    case MsgTy::CRITICAL:
        return "CRITICAL"sv;
    }
}

or to implement a stream inserter for the type:

template <typename CharT, typename Traits>
auto operator<<(std::basic_ostream<CharT, Traits>& o, MsgTy mt)
    -> std::basic_ostream<CharT, Traits>&
{
    switch (mt)
    {
    case MsgTy::OK:
        o << "OK";
    case MsgTy::WARNING:
        o << "WARNING";
    case MsgTy::ERROR:
        o << "ERROR";
    case MsgTy::CRITICAL:
        o << "CRITICAL";
    }

    return o;
}

or both.

Either option will be hundreds, if not thousands of times faster than using a std::map.

#define messaging(MsgTy, msg, log2file)

Okay, this is where the meat of the code is, and this is what you really want the focus to be on. Unfortunately, this is all wrong. This is exactly the way you should NEVER write a macro.

First let’s get the initial stuff out of the way. As I mentioned above, the convention in C++ (and even in C) is that macros should be in all caps. That’s not just for style, it’s because the unconstrained text replacement of the preprocessor is so dangerous. messaging isn’t exactly an uncommon word; it’s quite likely that it could be used for another identifer… with disastrous consequences. Using all caps accomplishes two things:

1. it alerts people to what they’re messing with; and
2. the only way it could every be used again is via a redefinition… which will trigger at least a warning.

The other problem with this preamble is that you’re using the message type enumeration’s type name as the parameter name. I can’t imagine why you’d think that’s a good idea. The only reason it works in this case is that you’re using an old-style enum. If you tried using a modern enum class, this whole macro would break.

There’s another issue buried in there: if the message type is ERROR or CRITICAL, you throw an exception. Okay, but the problem is the exception you throw is a std::string. If you run your program, it’s going to crash, because you catch a std::exception… but a std::string is not a std::exception. You probably want to either throw a std::runtime_error or, better, a custom exception type depending on whether it’s a ERROR or CRITICAL message.

Finally, you’ve made a critical macro error: you have repeated the argument(s). You have correctly wrapped them in parentheses, which helps prevent unexpected interactions with the surrounding code when expanded… but doesn’t help with the fact they they’re expanded multiple times. If you use an expression that changes the first argument in particular, who knows what could happen.

Overall, this is a terrible macro, for a number of reasons. First of all, it’s needlessly long. It injects almost 30 lines of code every time it’s used! In your main(), that try block that looks like it only has 4 lines in fact expands to well over 100 lines. That’s just ridiculous.

It’s also absurdly complex. Putting control flow in a macro is not just a “eh, it’s a thing you do”… it’s an absolute last resort. That’s really the golden rule of macros: keep them as simple as possible. That’s because they’re not only so hard to debug, but also because they’re expanded everywhere. They’re also exceptionally dangerous, so they should be written as simple as possible to avoid the need to ever tweak it in the future… as it is now, any time the requirements for how to log change, you have to mess with the macro code… which is playing with fire.

And a macro this complex just destroys your performance. Firstly, it will just absolutely trash your cache because all that machine code gets dumped everywhere the macro is used. (Although, if you’re lucky, and the macro is always used as you demonstrate, the compiler can probably remove most of those ifs.) But also, there are other side effects, too: for example, if messaging() were a function, profile-guided optimization would almost certainly mark the OK path as the hot path, and optimize the code accordingly… but that’s because there’s one if in one place; as a macro, that if gets repeated everywhere the macro is used, and it’s a different if every time, so PGO won’t help you much.

As it is, the macro’s code is in dire need of a rewrite, because it’s so repetitive, and there’s so much hard-coded in there (specifically the file name, over and over and over). But screwing with a macro is always a dangerous proposition; it’s MUCH riskier than refactoring a function. (It’s also sometimes much more frustrating, because the moment you touch a macro, you have to recompile everything, whereas a function can (sometimes!) be tweaked in isolation.)

And not only is it dodgy to use, hard to maintain, and inefficient… it’s also a terrible interface! Why is it necessary to specify whether you want the message written to the file or not on EVERY call? Isn’t assuming true a good default? With a function you could use an overload or a defaulted parameter for that, no problem.

At a bare minimum, to improve this macro, you should refactor as much as possible into functions:

#define MESSAGING(mt, msg, log_to_file) do { 
    auto const mt_ = (mt);

    if (mt_ == MsgTy::ok) 
        messaging_ok((msg), (log_to_file)); 
    else if (mt_ == MsgTy::warning) 
        messaging_warning((msg), (log_to_file)); 
    else if (mt_ == MsgTy::error) 
        messaging_error((msg), (log_to_file)); 
    else if (mt_ == MsgTy::critical) 
        messaging_critical((msg), (log_to_file)); 
} while (false)

Now you can fuss with the logic of each option safely.

Even better would be use static dispatch for this kind of thing. You could create a few types (ok_t, warning_t) and instances of those types (ok, warning), and then dispatch based on those:

struct ok_t {};
inline constexpr auto ok = ok_t{};
// etc. for the other message types

auto messaging(ok_t, std::string_view msg, bool log_to_file = true)
{
    std::cout << "OK: " << msg << std::endl; // endl to flush
    
    if (log_to_file)
    {
        auto out = std::ofstream{"log.txt", std::ios_base::app};
        out << "OK: " << msg;
        
        // or better yet, have a class that keeps the log file open
        // and just appends to it, rather than opening and closing
        // it repeatedly.
    }
}
// etc. for the other message types

messaging(ok, "Everything is good!");
messaging(warning, "Something isn't quite right!", false);
messaging(error, "Something went wrong!");
messaging(critical, "Something horribly went wrong!");

But that’s just one of dozens of techniques you can use to AVOID the use of macros… which is a much more useful skill to have in 2020.

In other words, all this has brought us back to the original point I wasn’t going to belabour. The best macro is the one you don’t write.

Questions

Are there any apparent issues with how I implemented the macro?

Yes, it is needlessly long and complex. Even for a non macro function, this is unnecessarily long and complex. It should be refactored into smaller functions for each of the four different behaviours.

Can there be any improvements made to it?

The best way to write a macro is: don’t.

I can’t conceive of why anyone would want to write a macro in 2020. Macros were a dirty hack when they were first created back in the 1970s. There may be a few very rare cases where you still need them, but by and large, if you can possibly solve a problem without macro, then THAT is the skill you should be exercising.

Is the intent clear on its usage?

Eeeh? Not really.

Is this the intended usage:

messaging(MsgTy::OK, 42, true);

Is this:

// won't work, but is it intended to?
messaging(MsgTy::OK, "a" << "b" << "c", true);

I'm also interested in any other feedback.

Don’t waste your time honing skills that nobody wants. Macros are old tech that are only, at best, tolerated, and only when there is absolutely no other option. The best skills you can learn regarding macros are ways to NOT use them. A programmer who is a master at writing macros, but because they don’t know all the ways they can avoid them always falls back to writing them, is less than useless to me on my projects.

Bottom line: Don’t waste your time. Getting good at writing macros helps no one. Instead, learn the techniques for AVOIDING macros. THOSE are skills that are actually useful in 2020 and beyond.