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

[Laurent Pinchart]

Hi Kieran,

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.

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

-- 
Regards,

Laurent Pinchart