Absolutely! I am not a friend of excessive macro use either. Using macros is convenient for class declarations to reduce type work (e.g. the TYPESYSTEM_HEADER* macros similar to Qt's Q_OBJECT) but function-like macros are already very questionable and IMO don't outweigh their disadvantages.In general, it seems that macro usage should be avoided in minimized.
I agree. Using a function would be the better approach.Specifically, my issue with the FC_CONSOLE_FMT macro is that it makes assumptions about the context in which it will be used - in assumes that it is used within a function that has a pMsg member variable. In my opinion, this is poorly done, makes the code difficult to read, and can likely lead to difficult to find bugs in the future.
This makes no sense because the method name "Message" says that it's a normal message and neither a warning, an error or log message. So, adding Notification to the function parameter list makes no sense.Then you could replace each FC_CONSOLE_FMT(message, which) with ConsoleSingleton::Message(pMsg, Notification::WARNING) (etc...).
A private function can be added that looks like:I'm not saying this is the most optimal way to do this ...there may be other opportunities to clean up the code a bit. If others agree that a change like this make sense, I don't mind working on it to put a PR together.
Code: Select all
void ConsoleSingleton::notify(std::function<void(const char*)> notify_meth, ConsoleSingleton::FreeCAD_ConsoleMsgType msgType, const char *pMsg, va_list namelessVars)
{
char format[BufferSize];
format[sizeof(format)-4] = '.';
format[sizeof(format)-3] = '.';
format[sizeof(format)-2] = '\n';
format[sizeof(format)-1] = 0;
const unsigned int format_len = sizeof(format)-4;
vsnprintf(format, format_len, pMsg, namelessVars);
format[sizeof(format)-5] = '.';
if (connectionMode == Direct)
notify_meth(format);
else
QCoreApplication::postEvent(ConsoleOutput::getInstance(), new ConsoleEvent(msgType, format));
}
Code: Select all
void ConsoleSingleton::Warning( const char *pMsg, ... )
{
va_list namelessVars;
va_start(namelessVars, pMsg);
notify(boost::bind(&ConsoleSingleton::NotifyWarning, this, _1), MsgType_Wrn, pMsg, namelessVars);
va_end(namelessVars);
}
But code duplication would not be a lot when just copying & adjusting the macro code to the corresponding functions.