Code review comment for lp:~phablet-team/media-hub/add-logger

Revision history for this message
Jim Hodapp (jhodapp) wrote :

> I have some concerns with this MP:
>
> * I do not really like moving to the printf way of doing things. Why should be
> drop type safeness? It would be better to use the boost logger more "natively"
> with "<<" operators, changing the wrapper class to something similar to what
> Qt does (qDebug() << message, etc.). Even more, using printf way in the end we
> need to resort to using stringstream in several places where it was not needed
> before.

Boost still maintains complete type safety with using the printf style. See the Boost::Log docs about this if you have any questions. This private Boost::Log interface implementation comes from tvoss and we're starting to standardize on it. Not sure why it doesn't use streams but that's outside anything I can really control without rewriting the private logger interface.

>
> * I still see cout and cerr being used in many places. But please do not
> change that yet until we have an agreement on how the class Logger should be
> used.

There are some that will need to remain, but otherwise I've made sure that all superfluous ones have now been removed.

>
> * I do not think we should introduce functions in the Utils namespace that we
> do not use. Specially taking into account that I would prefer to avoid using
> the only one that seems to be used, Sprintf :)

Fixed

>
> * Line sizes in new files are > 100 in many cases

I tried not to do that, any specific ones?

>
> There are also a couple of inline comments.

« Back to merge proposal