Best practise?

[berniev]

Code: Select all

#define FC_LOGLEVEL_DEFAULT -1
#define FC_LOGLEVEL_ERR 0
#define FC_LOGLEVEL_WARN 1
#define FC_LOGLEVEL_MSG 2
#define FC_LOGLEVEL_LOG 3
#define FC_LOGLEVEL_TRACE 4

#define _FC_LOG_LEVEL_INIT(_name,_tag,...) \
    static Base::LogLevel _name(_tag,## __VA_ARGS__);

#ifndef FC_LOG_INSTANCE
#   define FC_LOG_INSTANCE _s_fclvl
#endif

#define FC_LOG_LEVEL_INIT(_tag,...) \
    _FC_LOG_LEVEL_INIT(FC_LOG_INSTANCE, _tag, ## __VA_ARGS__)

#define __FC_PRINT(_instance,_l,_func,_msg,_file,_line) do{\
    if(_instance.isEnabled(_l)) {\
        std::stringstream _str;\
        _instance.prefix(_str,_file,_line) << _msg;\
        if(_instance.add_eol) \
            _str<<std::endl;\
        Base::Console()._func(_str.str().c_str());\
        if(_instance.refresh) Base::Console().Refresh();\
    }\
}while(0)

#define _FC_PRINT(_instance,_l,_func,_msg) __FC_PRINT(_instance,_l,_func,_msg,__FILE__,__LINE__)

#define FC_MSG(_msg) _FC_PRINT(FC_LOG_INSTANCE,FC_LOGLEVEL_MSG,NotifyMessage,_msg)
#define FC_WARN(_msg) _FC_PRINT(FC_LOG_INSTANCE,FC_LOGLEVEL_WARN,NotifyWarning,_msg)
#define FC_ERR(_msg) _FC_PRINT(FC_LOG_INSTANCE,FC_LOGLEVEL_ERR,NotifyError,_msg)
#define FC_LOG(_msg) _FC_PRINT(FC_LOG_INSTANCE,FC_LOGLEVEL_LOG,NotifyLog,_msg)
#define FC_TRACE(_msg) _FC_PRINT(FC_LOG_INSTANCE,FC_LOGLEVEL_TRACE,NotifyLog,_msg)

#define _FC_MSG(_file,_line,_msg) __FC_PRINT(FC_LOG_INSTANCE,FC_LOGLEVEL_MSG,NotifyMessage,_msg,_file,_line)
#define _FC_WARN(_file,_line,_msg) __FC_PRINT(FC_LOG_INSTANCE,FC_LOGLEVEL_WARN,NotifyWarning,_msg,_file,_line)
#define _FC_ERR(_file,_line,_msg) __FC_PRINT(FC_LOG_INSTANCE,FC_LOGLEVEL_ERR,NotifyError,_msg,_file,_line)
#define _FC_LOG(_file,_line,_msg) __FC_PRINT(FC_LOG_INSTANCE,FC_LOGLEVEL_LOG,NotifyLog,_msg,_file,_line)
#define _FC_TRACE(_file,_line,_msg) __FC_PRINT(FC_LOG_INSTANCE,FC_LOGLEVEL_TRACE,NotifyLog,_msg,_file,_line)

#define FC_XYZ(_pt) '('<<(_pt).X()<<", " << (_pt).Y()<<", " << (_pt).Z()<<')'
#define FC_xy(_pt) '('<<(_pt).x<<", " << (_pt).y<<')'
#define FC_xyz(_pt) '('<<(_pt).x<<", " << (_pt).y<<", " << (_pt).z<<')'

#ifndef FC_LOG_NO_TIMING
#   define FC_TIME_CLOCK high_resolution_clock
#   define FC_TIME_POINT std::chrono::FC_TIME_CLOCK::time_point
#   define FC_DURATION std::chrono::duration<double>

#   define _FC_TIME_INIT(_t) _t=std::chrono::FC_TIME_CLOCK::now()
#   define FC_TIME_INIT(_t) FC_TIME_POINT _FC_TIME_INIT(_t)
#   define FC_TIME_INIT2(_t1,_t2) FC_TIME_INIT(_t1),_t2=_t1
#   define FC_TIME_INIT3(_t1,_t2,_t3) FC_TIME_INIT(_t1),_t2=_t1,_t3=_t1

#   define _FC_DURATION_PRINT(_l,_d,_msg) \
        FC_##_l(_msg<< " time: " << _d.count()<<'s');

#   define FC_DURATION_MSG(_d,_msg) _FC_DURATION_PRINT(MSG,_d,_msg)
#   define FC_DURATION_LOG(_d,_msg) _FC_DURATION_PRINT(LOG,_d,_msg)
#   define FC_DURATION_TRACE(_d,_msg) _FC_DURATION_PRINT(TRACE,_d,_msg)

#   define _FC_TIME_PRINT(_l,_t,_msg) \
        _FC_DURATION_PRINT(_l,Base::GetDuration(_t),_msg);

#   define FC_TIME_MSG(_t,_msg) _FC_TIME_PRINT(MSG,_t,_msg)
#   define FC_TIME_LOG(_t,_msg) _FC_TIME_PRINT(LOG,_t,_msg)
#   define FC_TIME_TRACE(_t,_msg) _FC_TIME_PRINT(TRACE,_t,_msg)

#   define FC_DURATION_DECLARE(_d) FC_DURATION _d
#   define FC_DURATION_DECLARE2(_d,_d1) FC_DURATION_DECLARE(_d),_d1
#   define FC_DURATION_DECLARE3(_d,_d1) FC_DURATION_DECLARE2(_d,_d1),_d2

#   define FC_DURATION_INIT(_d) _d=FC_DURATION(0)
#   define FC_DURATION_INIT2(_d,_d1) _d=_d1=FC_DURATION(0)
#   define FC_DURATION_INIT3(_d,_d1,_d2) _d=_d1=_d2=FC_DURATION(0)

#   define FC_DURATION_DECL_INIT(_d) FC_DURATION _d(0)
#   define FC_DURATION_DECL_INIT2(_d,_d1) FC_DURATION_DECL_INIT(_d),_d1(0)
#   define FC_DURATION_DECL_INIT3(_d,_d1) FC_DURATION_DECL_INIT2(_d,_d1),_d3(0)

#   define FC_DURATION_PLUS(_d,_t) _d += Base::GetDuration(_t)

#else //FC_LOG_NO_TIMING
#   define FC_TIME_POINT
#   define _FC_TIME_INIT(...) do{}while(0)
#   define FC_TIME_INIT(...) do{}while(0)
#   define FC_TIME_INIT2(...) do{}while(0)
#   define FC_TIME_INIT3(...) do{}while(0)
#   define _FC_DURATION_PRINT(...) do{}while(0)
#   define _FC_TIME(_t) do{}while(0)
#   define FC_DURATION_PRINT(...) do{}while(0)
#   define FC_DURATION
#   define FC_DURATION_INIT(...) do{}while(0)
#   define FC_DURATION_INIT1(...) do{}while(0)
#   define FC_DURATION_INIT2(...) do{}while(0)
#   define FC_DURATION_DECLARE(...)
#   define FC_DURATION_DECLARE1(...)
#   define FC_DURATION_DECLARE2(...)
#   define FC_DURATION_DECL_INIT(...) do{}while(0)
#   define FC_DURATION_DECL_INIT2(...) do{}while(0)
#   define FC_DURATION_DECL_INIT3(...) do{}while(0)
#   define FC_DURATION_PLUS(...) do{}while(0)

#endif //FC_LOG_NO_TIMING

What do you think?
BestPractise / GoodPractise / Acceptable / Dodgy / Unacceptable?

[openBrain]

Life is relative. So I will quote it as "acceptable".

Sure it's far from ideal though. If you aim at refactoring this kind of things, will show you the worst I probably ever found in FreeCAD code till now.
The "TreeParams" class :
https://github.com/FreeCAD/FreeCAD/blob ... #L508-L556
https://github.com/FreeCAD/FreeCAD/blob ... p#L90-L198

Please ensure you have an healthy heart before clicking the links.

[berniev]

the worst I probably ever found in FreeCAD code till now.
The "TreeParams" class

Same, Same!

[openBrain]

Same, Same!

If you feel brave enough, I'd be interested in knowing how you'd tackle this with modern C++.

[berniev]

If you feel brave enough, I'd be interested in knowing how you'd tackle this with modern C++.

Can we come back to this when others have had a chance to comment?

[openBrain]

If you feel brave enough, I'd be interested in knowing how you'd tackle this with modern C++.

Can we come back to this when others have had a chance to comment?

On this one, @wmayer already agreed it needs some refactoring https://github.com/FreeCAD/FreeCAD/pull ... 1100208691

[berniev]

If you feel brave enough, I'd be interested in knowing how you'd tackle this with modern C++.

Can we come back to this when others have had a chance to comment?

On this one, @wmayer already agreed it needs some refactoring https://github.com/FreeCAD/FreeCAD/pull ... 1100208691

I have learnt (finally) that an early opinion attracts a bunch of rubbish. So if you don't mind I'll allow others to have an objective say. But for sure we'll come back to this.

[wandererfan]

Code: Select all

#define FC_LOGLEVEL_DEFAULT -1

To me, #define for a magic number is ok. The rest is obfuscation.

[wmayer]

What do you think?
BestPractise / GoodPractise / Acceptable / Dodgy / Unacceptable?

For me it's between Dodgy and Unacceptable. These nested C macros are a PITA and almost unmaintainable code and this special case is not even strict ISO C++ compliant. The sooner this will be converted into clean C++ code the better.

The excessive use of C macros over the last couple of years should be reduced to a minimum again.

[berniev]

Thanks for the comments.

I think that @wmayer (as usual ! ) says it best.

I stumbled on this code and nearly choked. How could this 1995 style stuff still be in the codebase?

Sadly, this, and that mentioned by @openBrain were both merged just three years ago. I hope the topo merge doesn't pull in a bunch more.

In terms of refactoring, I suggest getting rid of macros wherever possible.

Recently I found a humble slash that made no sense. Finally noticed it was a macro (in Boost, from memory). In single simple character the color difference was almost impossible to notice. Does some string concat stuff. Really 'clever !', but really?.

And then there's the macros that cause warnings all through build because the code in them has not been modernised. It appears nobody is game to touch them. An IDE sees plain text, no more. PITA.

I did a PR 7473 exploring use of constexpr (since C++11).

Any and all feedback appreciated.