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:
- it alerts people to what they’re messing with; and
- 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.
