Resolve "QObject* cannot be logged (operator<< is ambiguous )" - [merged] #93

Closed rainman110 closed 10 months ago

In GitLab by @mariusalexander on Oct 28, 2022, 08:28

Merges 31-qobject-cannot-be-logged-operator-is-ambiguous -> master

Closes #31

Description

Template based operator<<

I have implemented two template based operator<<:

Only accepts types that have a global operator<<(QDebug&, <type>) implemented
Uses the QDebug::operator<<(<type>). This is only enabled if the first does not exists

This should allow gt::log::Stream to work with any types that QDebug can work with as well.

Support for additional types

Additionally if added support for std::string, which is not present in QDebug.

Custom Operator<<

It should also be possible to define custom stream operators.

However there is still an issue with container types. Consider the following example:

struct MyType {};

gt::log::Stream operator<<(gt::log::Strean& s, MyType const& t) {
   return s << "MyType";
}

In this case MyType can only be logged by our gt::log::Stream class. However say we have a QVector<MyType>, this types cannot be logged, since we use the operator<< of QDebug for logging container types. QDebug does not have a operator<< for MyType. We therefore need to handle the these cases differently, maybe in a separate issue?

In GitLab by @mariusalexander on Oct 28, 2022, 08:30

added 1 commit

0680bd75 - fixed logstream for qobject ptrs

Compare with previous version

In GitLab by @mariusalexander on Oct 28, 2022, 08:30

marked this merge request as ready

In GitLab by @mariusalexander on Oct 28, 2022, 08:31

@rainman110 Please approve. There was a bug in the logstream that prohibited the printing of QObjects*

In GitLab by @mariusalexander on Oct 28, 2022, 15:39

added 2 commits

8cea6dad - draft of an operator<< for all QDebug compatible objects
93477eca - fix

Compare with previous version

In GitLab by @rainman110 on Oct 28, 2022, 22:00

Wow... What template magic! Kudos

In GitLab by @rainman110 on Oct 28, 2022, 22:00

approved this merge request

In GitLab by @rainman110 on Oct 29, 2022, 05:31

Please test this manually on Linux as long as our ci is currently down.

In GitLab by @mariusalexander on Nov 1, 2022, 16:34

added 1 commit

5212ba1e - extended logging by using global and internal QDebug shift operators, added a bunch of unittests

Compare with previous version

In GitLab by @rainman110 on Nov 1, 2022, 16:59

Awesome last commit ❤️

Great, that you can now automatically delegate everything to qdebug, that is supported by qdebug.

In GitLab by @mariusalexander on Nov 1, 2022, 17:03

Thanks :smile:! I've also commented out a few operator<< for types that work now "automatically". We can have a look at it tomorrow, maybe the pipeline will work again then as well.

In GitLab by @rainman110 on Nov 2, 2022, 08:15

How do we proceed? We need to test this and then make a release as quick as possible, right?

In GitLab by @mariusalexander on Nov 2, 2022, 08:31

added 2 commits

d320b401 - added unittests for logging line numbers
4ff0e80e - cleanup of unused loggin operators

Compare with previous version

In GitLab by @mariusalexander on Nov 2, 2022, 11:04

added 1 commit

2297b8ec - fix

Compare with previous version

In GitLab by @mariusalexander on Nov 2, 2022, 11:04

Yes I think so :+1:

In GitLab by @mariusalexander on Nov 2, 2022, 11:05

If the pipeline succeeds we should merge this and make a release

In GitLab by @mariusalexander on Nov 2, 2022, 11:09

added 1 commit

628c263a - fix 2

Compare with previous version

In GitLab by @mariusalexander on Nov 2, 2022, 11:12

added 1 commit

fb73d4df - fix 3

Compare with previous version

In GitLab by @mariusalexander on Nov 2, 2022, 12:50

added 1 commit

d7501ad9 - final fix

Compare with previous version

In GitLab by @mariusalexander on Nov 2, 2022, 12:53

added 1 commit

be1ae97c - cleanup

Compare with previous version

In GitLab by @rainman110 on Nov 2, 2022, 12:56

In this case MyType can only be logged by our gt::log::Stream class. However say we have a QVector<MyType>, this types cannot be logged, since we use the operator<< of QDebug for logging container types. QDebug does not have a operator<< for MyType. We therefore need to handle the these cases differently, maybe in a separate issue?

Yes, lets not overdo this here. As long as all our code / modules compile and the tests are successful, this should be enough for now. We can still implement a custom template vector specialization right?

In GitLab by @rainman110 on Nov 2, 2022, 12:57

Great work by the way

In GitLab by @rainman110 on Nov 2, 2022, 12:59

Commented on tests/unittests/test_loglinenumbers.cpp line 21

:thumbsup:

In GitLab by @rainman110 on Nov 2, 2022, 12:59

Commented on tests/unittests/test_types.cpp line 32

Can we remove the qDebug statements?

In GitLab by @rainman110 on Nov 2, 2022, 12:59

Commented on tests/unittests/test_types.cpp line 46

and here?

In GitLab by @rainman110 on Nov 2, 2022, 13:00

Commented on tests/unittests/test_types.cpp line 60

and here?

In GitLab by @rainman110 on Nov 2, 2022, 13:00

Commented on tests/unittests/test_types.cpp line 60

and all the others...

In GitLab by @rainman110 on Nov 2, 2022, 13:00

Commented on tests/unittests/test_types.cpp line 60

otherwise.... add qDebug() << log to the TearDown method

In GitLab by @mariusalexander on Nov 2, 2022, 13:05

FYI: The issue on Linux was that QVariant has ambigous overloads for long and long long (i.e.g when logging gtInfo() << long{42};. However I was confused, because we have implemented dedicated methods for long and long long and I didnt unterstand why the compiler was looking into QVariant.

But I think the compiler will first search for all candidate functions, which includes the deciated long and long long of gt::log::Stream and both templated methods and thus finds the Qvariant overloads. Becuase the compiler then sees the ambigious overload it exists as SFINAE does not apply here (?). Does this make sense?

Thats why if added the if_not_integral alias/enable_if

In GitLab by @mariusalexander on Nov 2, 2022, 13:06

Commented on tests/unittests/test_types.cpp line 60

I will, I think its good if we also see the actual thing beeing logged instead of just the tests passing :)

In GitLab by @mariusalexander on Nov 2, 2022, 13:07

Commented on tests/unittests/test_loglinenumbers.cpp line 21

I wanted to implement tests for GT_LOG_QOUTE and GT_LOG_NO_SPACE, but I think this must be defined globally. Could be an issue for another time.

In GitLab by @mariusalexander on Nov 2, 2022, 13:08

Yes I think so, its all defined in the headers anyway :+1:

In GitLab by @rainman110 on Nov 2, 2022, 13:10

I think this makes sense. The compiler will always search for an exact match before trying to search for implicit conversions. It still puzzles me, why we still have big differences between gcc and msvc when it comes to templates. This is also why I insisted to check for the linux build ;)

In GitLab by @mariusalexander on Nov 2, 2022, 13:16

Commented on tests/unittests/test_types.cpp line 32

changed this line in version 10 of the diff

In GitLab by @mariusalexander on Nov 2, 2022, 13:16

Commented on tests/unittests/test_types.cpp line 46

changed this line in version 10 of the diff

In GitLab by @mariusalexander on Nov 2, 2022, 13:16

Commented on tests/unittests/test_types.cpp line 60

changed this line in version 10 of the diff

In GitLab by @mariusalexander on Nov 2, 2022, 13:16

added 1 commit