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

[Kieran Bingham]

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 ...



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

-- 
Regards
--
Kieran