[libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up variable names to be consistent

[Kieran Bingham]

Hi Jacopo,

On 23/09/2020 12:39, Jacopo Mondi wrote:
> /me dives into bikeshedding

<snip>

>>> @@ -134,11 +134,11 @@ private:
>>>  	 * We count frames to decide if the frame must be hidden (e.g. from
>>>  	 * display) or mistrusted (i.e. not given to the control algos).
>>>  	 */
>>> -	uint64_t frame_count_;
>>> +	uint64_t frameCount_;
>>
>> If we're doing clean up - I always prefer to see a newline before a
>> comment line.

Note here I state 'before a comment'

>> To me it makes the commented block clearer - but I don't know if there's
>> a 'rule' on it.
>>
> 
> I'm sorry but
> 
>         /*
>          * A comment describing a variable is better places as close
>          * as possible near to that variable, isn't it ?
>          */
>          int whatAFancyVariable;
> 
>          /* Or do you think this variable is not fancy and an empty line is better?  */
> 

Here you seem to be implying 'after' a comment...

>          int notAFancyVariable;
> 
> Ok, enough useless discussions from me :)
> 

I ... 'think' you mis-interpreted what I said, or I was not clear. I was
probably not clear ;-)

I think that this:

	/* A variable group. */
	bool variable;
	int quantity;

	/*
	 * We count frames to decide if the frame must be hidden (e.g.
	 * from display) or mistrusted (i.e. not given to the control
	 * algos).
	 */
	uint64_t frameCount_;

	/* For checking the sequencing of Prepare/Process calls. */
	uint64_t checkCount_;

	/* How many frames we should avoid running control algos on. */
	unsigned int mistrustCount_;

is better than this:


	/* A variable group */
	bool variable;
	int quanty;
	/*
	 * We count frames to decide if the frame must be hidden (e.g.
	 * from display) or mistrusted (i.e. not given to the control
	 * algos).
	 */
	uint64_t frameCount_;
	/* For checking the sequencing of Prepare/Process calls. */
	uint64_t checkCount_;
	/* How many frames we should avoid running control algos on. */
	unsigned int mistrustCount_;

Maybe one day I'll finish my comment formatter in the checkstyle script ;-)


>>
>>>  	/* For checking the sequencing of Prepare/Process calls. */
>>> -	uint64_t check_count_;
>>> +	uint64_t checkCount_;
>>
>> i.e. here too.
>>
>>>  	/* How many frames we should avoid running control algos on. */
>>> -	unsigned int mistrust_count_;
>>> +	unsigned int mistrustCount_;
>>
>> and so on.
>>
>>>  	/* LS table allocation passed in from the pipeline handler. */
>>>  	FileDescriptor lsTableHandle_;
>>>  	void *lsTable_;

<snip>
-- 
Regards
--
Kieran