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

[Naushir Patuck]

Hi Kieran and Jacopo,

Thank you both for the review comments.

On Wed, 23 Sep 2020 at 12:45, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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_;

Yes, I agree the latter block is less readable than the former.  I
will update this patch with some extra whitespace where needed.

Regards,
Naush


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