[libcamera-devel] [PATCH] libcamera: Add debug printouts

[Laurent Pinchart]

Hello again,

On Tuesday, 15 January 2019 15:28:45 EET Laurent Pinchart wrote:
> On Tuesday, 15 January 2019 13:00:50 EET Kieran Bingham wrote:
> > On 15/01/2019 02:02, Laurent Pinchart wrote:
> > > Hello,
> > 
> > <snip>
> > 
> > >>> -	if (pipe->match(enumerator))
> > >>> +	if (pipe->match(enumerator)) {
> > >>> +		LOG(Debug) << "Pipeline handler: \"" << name << "\" matched";
> > > 
> > > And removing the ":" here too.
> > > 
> > >> (not for this patch - just ideas out loud)
> > >> 
> > >> Can/Should we make a function/macro called
> > >> 
> > >> 	 quoted((os), (_s))
> > >>  
> > >>  which will do:
> > >>   os << ("\"" << (_s) << "\"")
> > > 
> > > I suppose this would be
> > > 
> > > #define quote(s)		"\"" << s << "\""
> > 
> > Ah yes - I was overcomplicating already :)
> > 
> > >> LOG(Debug) << "Pipeline handler: \"" << name << "\" matched";
> > >> LOG(Debug) << "Pipeline handler: " << quoted(name) << " matched";
> > > 
> > > Wouldn't it reduce readability by hiding a simple construct behind a
> > > macro
> > > ? I know it's tempting to avoid writing the escape sequence...
> > 
> > Does it? I guess it's subjective.
> > 
> > It is a simple construct yes, but IMO the escaping of the {"} hinders
> > 
> > readability more. A quick glance here:
> >   : \"" << name << "\" matched
> > 
> > could look like we are sending the string
> > 
> >   { name "\" matched } as if it was a path with backslashes or such...
> > 
> > (Yes I'm aware that the example above is taken slightly out of context,
> > and it was done so purposefully to represent 'a quick glance')...
> > 
> > 
> > Also a helper here would ensure that quotes are correctly terminated, as
> > demonstrated in Jacopo's patch f1caaaf387 ("libcamera: Debug printouts
> > fixes") which adds in a missing final "\"";
> > 
> > +	LOG(Debug) << "Successful match for media device \""
> > +		   << dev->driver() << "\"";
> > 
> > Although in that particular case - I think that log statement could have
> > been on one line where it would then be more obvious too.
> > 
> > 	LOG(Debug) << "Successful match for media device "
> > 	
> > 		   << quoted(dev->driver());
> > 
> > Anyway - it was just an idea/suggestion, I'm not tied to it ...
> 
> If you submit a patch I won't reject it :-) I would however prefer a
> function instead of a macro, in order to avoid claiming such a generic name
> in the global namespace. One option would be, in log.h,
> 
>  	LogSeverity severity_;
>  };
> 
> +namespace log {
> +
> +static inline std::string quoted(const std::string &name)
> +{
> +	return "\"" + name + "\"";
> +}
> +
> +} /* namespace log */
> +
> 
>  #define LOG(severity) LogMessage(__FILE__, __LINE__,
> Log##severity).stream()
> 
> (Whether we want a log namespace there is debatable). The function would be
> used as
> 
> 	LOG(Debug) << "Print a quoted " << log::quoted(name);
> 
> I haven't studied whether there could be efficiency concerns due to string
> concatenation.

I forgot to mention that a simple solution to all this could be to use single 
quotes instead of double quotes.

	LOG(Debug) << "Print a quoted '" << name << "'";

> > >>>  		return pipe;
> > >>> 
> > >>> +	}
> > >>> 
> > >>>  	delete pipe;
> > >>>  	return nullptr;
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


-- 
Regards,

Laurent Pinchart