Code review of merged Link3 branch

[wmayer]

In general, it seems that macro usage should be avoided in minimized.

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.

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.

I agree. Using a function would be the better approach.

Then you could replace each FC_CONSOLE_FMT(message, which) with ConsoleSingleton::Message(pMsg, Notification::WARNING) (etc...).

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.

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.

A private function can be added that looks like:

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));
}

and the calling functions e.g. Warning would be:

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 in terms of consistency it's better to put the logic whether to call NotifyMessage, NotifyWarning, ... inside the notify() function because the MsgType (here MsgType_Wrn) is already defined by the calling instance.

But code duplication would not be a lot when just copying & adjusting the macro code to the corresponding functions.

[ezzieyguywuf]

Absolutely! I am not a friend of excessive macro use either.

Glad to see I'm not the only one enthusiastic about this topic.

Using macros is convenient for class declarations to reduce type work (e.g. the TYPESYSTEM_HEADER* macros similar to Qt's Q_OBJECT)

100% agree. In general, if I see a coding pattern or technique used on a mature project such a Qt (which has fantastic documentation, truly a standard I strive for in my own projects), it goes a long way towards convincing me that it is a "good" use of such a technique.

Then you could replace each FC_CONSOLE_FMT(message, which) with ConsoleSingleton::Message(pMsg, Notification::WARNING) (etc...).

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.

Ok, after spending a bit more time reading through the ConsoleSingleton code, I think I can agree with this....

A private function can be added that looks like:

Yes, I think this is a better approach.

and the calling functions e.g. Warning would be:

This is the logical conclusion of your recommendation.

However, I still don't like the code duplication necessary to handle the varargs.

What if we update the code using modern c++'s variadic template functions into something like this (untested, just trying to show the point):

Code: Select all

//Console.h
//<snip>
namespace Base{
    class BaseExport ConsoleSingleton{
        public:
            //<snip>
            template<typename... Args>
            void Message(const std::string& message, Args... args) const override;
            //etc... for other notification types
            //<snip>
        private:
            //<snip>
            class enum Level{
                MESSAGE,
                WARNING,
                ERROR,
                LOG
            }
            //Using the method described here https://stackoverflow.com/a/42511919/4608626
            template<typename... Args>
            void Notify(const std::string& message, Level which, Args&& ... args) const;
            //<snip>
    };
}
//<snip>
//Console.cpp
//<snip
using Base;
template<typename... Args>
void ConsoleSingleton::Message(const std::string& message, Args... args) const 
{
    this->Notify(message, Level::MESSAGE, args...);
}

template<typename... Args>
void ConsoleSingleton::Notify(const std::string& message, Level which, Args&& ... args) const
{
    // First, we need to determine what size our new string will be
    auto size = std::snprintf(nullptr, 0, message.c_str(), std::forward<Args>(args)...);
    // Next, we create a string with the appropriate amount of memory reserved.
    // Note that we add the terminating character
    std::string output(size + 1, '\0');
    // Now we can use sprintf as usual
    std::sprintf(&output[0], message.c_str(), std::forward<Args>(args)...);
    // Use a range-based for-loop.
    // NOTE: should _aclObservers use a smart pointer?
    for(ConsoleObserver* observer : _aclObservers)
    {
        switch (which)
        {
            case Level::MESSAGE:
                if(observer->bMsg):
                    observer->Message(output.c_str());
                break;
            case Level::WARNING:
                if(observer->bWrn):
                    observer->Warning(output.c_str());
                break;
            case Level::ERROR:
                if(observer->bErr):
                    observer->Error(output.c_str());
                break;
            case Level::LOG:
                if(observer->bLog):
                    observer->Log(output.c_str());
                break;
            default:
                break;
        }
    }
}
//<snip

[wmayer]

However, I still don't like the code duplication necessary to handle the varargs.

It's because it's not possible to forward a variable number of arguments to another function with a variable number of arguments.

What if we update the code using modern c++'s variadic template functions into something like this (untested, just trying to show the point):

In long-term it's good to get rid of old (and latent insecure) C techniques in modern C++ code.

The Message() method (and the others) implement the same semantic as the printf() function of C and I don't think you can implement the exact equivalent with variadic templates because it's not so easy to detect all the combinations of format specifiers inside the strings.
However, here is offered something similar (at the end): https://en.cppreference.com/w/cpp/langu ... meter_pack

[ezzieyguywuf]

it's not possible to forward a variable number of arguments to another function with a variable number of arguments.

Is this not what std::forward is intended to do?

The Message() method (and the others) implement the same semantic as the printf() function of C

Yes, I am aware of this. It does add some complexity to the code-base, but I understand the need/desire to keep this functionality around.

I don't think you can implement the exact equivalent with variadic templates

Perhaps the actual technique I ended up using in the code snippet is not a "variadic template", but I do believe it solves the problem of using printf semantics. Look specifically at the ConsoleSingleton::Notify method, which uses a technique described on this SO answer to maintain the printf syntax.

Would it be helpful if I pulled all this together into a PR to review/discuss?

[realthunder]

I have submitted a PR with fixes for most of the issues mentioned in this thread except a few requiring further discussion. Also the setVisible() enum suggestion will be included in a separate future PR.

Regarding macros, well, I admittedly overused a bit in my code. But I would argue that it definitely has its place in modern C++, just look at boost code. And the fact that you are considering using std::function or bind for such a straightforward task somewhat proves my point. FC_CONSOLE_FMT is a private macro defined in a cpp file. It's usage is very specific. I don't think its inappropriate here.

[wmayer]

But I would argue that it definitely has its place in modern C++, just look at boost code.

boost is probably not the very best example in this context because it tries to support very old compiler versions and therefore the vast majority of macros are needed. So, it has not much to do with modern C++ as such.
If you look e.g. at pybind11 which is a similar Python wrapper like boost.python the devs there require modern C++ compilers and so they could get rid of all the ugly macro stuff.

And the fact that you are considering using std::function or bind for such a straightforward task somewhat proves my point.

Yes, this causes an overhead and is less efficient than the macro. On the other hand it's really difficult to understand this specific macro and if someone wants to modify it in the future the likelihood is rather high that he unintentionally breaks something.

[wmayer]

Is this not what std::forward is intended to do?

How can this be combined with a C function with variable number of arguments which has a signature like this?

Code: Select all

void function(const char* fmt, ...);

The function does not tell anything about the types of the further arguments and thus you cannot pass a type to std::forward or do I miss something?

[ezzieyguywuf]

How can this be combined with a C function with variable number of arguments which has a signature like this?

It cannot. The C-style function must be updated as such:

Code: Select all

template<typename... Args>
void function(const char* fmt, Args... args);

I'm going to try to put together a compilable working example of this concept, to try to make things more clear.

[ezzieyguywuf]

Here is a minimal working example of the concept I'm describing.

It can be compiled using and run

Code: Select all

git clone https://gitlab.com/ezzieyguywuf_cpp_examples/variablearguments.git
cd VariableArguments
mkdir build
cd build
cmake ..
make
./varargs

Example output:

Code: Select all

Log: Hello, this is a regular message.
Log: Hello, this is 10 messages.

[wmayer]

Do I have to create an account just to look at this branch?