[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.

[Kieran Bingham]

Quoting Nicholas Roth (2022-10-27 22:52:46)
> Hey, so now I'm *really* confused by the post-commit. It almost seems like
> it wants me to use a mix of spaces and tabs, which I've never seen before.
> Let me know what you make of this:
> 
> I start with this suggestion:
> --- src/ipa/ipu3/algorithms/agc.cpp
> +++ src/ipa/ipu3/algorithms/agc.cpp
> @@ -101,7 +101,7 @@
>         /* Configure the default exposure and gain. */
>         activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);
>         activeState.agc.exposure = 10ms /
> -
> std::chrono::duration(configuration.sensor.lineDuration);
> +
>  std::chrono::duration(configuration.sensor.lineDuration);
> 
>         frameCount_ = 0;
>         return 0;
> 
> Then when I delete a tab as suggested, I end up with this:
> --- src/ipa/ipu3/algorithms/agc.cpp
> +++ src/ipa/ipu3/algorithms/agc.cpp
> @@ -101,7 +101,7 @@
>         /* Configure the default exposure and gain. */
>         activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);
>         activeState.agc.exposure = 10ms /
> -
> std::chrono::duration(configuration.sensor.lineDuration);
> +
>  std::chrono::duration(configuration.sensor.lineDuration);
> 
>         frameCount_ = 0;
>         return 0;
> 
> If a mix of tabs and spaces is what's standard for us, I'll do it. I just
> thought to ask first since it would be the first time I've seen a project
> with this convention.

There 'are' a mix of spaces and tabs - but it's about alignment.

It's really hard to determine from your mail above which is
space/tabs/indent due to the mail wraps. but hopefully this can lay it
out. clang-format gets it right most of the time (which is what the post
commit hook is using) but it always needs a quick review because
sometimes clang-format does things that are just plain stupid or less
readable. So it's there for guidance rather than specification.


~######~ == Tab (8 per tab)
.....    == Spaces ( 1 per space )


So these single indents for example are all tabs:

~######~if (cfg.size != size) {
~######~~######~LOG(UVC, Debug)
~######~~######~~######~<< "Adjusting size from " << size << " to " << cfg.size;
~######~~######~status = Adjusted;
~######~}

But if there was something which needs to be directly aligned below a
character on the previous line, then it's tabs to fill the gap, with
spaces for the alignment:

For example on this function prototype, the paramaters are aligned:

~######~int exportFrameBuffers(Camera *camera, Stream *stream,
~######~~######~~######~.......std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;

As I understand it, that's pretty close to the style in the kernel:

  https://www.kernel.org/doc/html/v4.10/process/coding-style.html


And this is the extract from our https://libcamera.org/coding-style.html#coding-style-guidelines

  Coding Style

Even if the programming language in use is different, the project
embraces the Linux Kernel Coding Style with a few exception and some C++
specificities.

In particular, from the kernel style document, the following section are
adopted:

    1 “Indentation”
    2 “Breaking Long Lines” striving to fit code within 80 columns and accepting up to 120 columns when necessary
    3 “Placing Braces and Spaces”
    3.1 “Spaces”
    8 “Commenting” with the exception that in-function comments are not always un-welcome.


> 
> Thanks,
> -Nicholas
> 
> On Thu, Oct 27, 2022 at 6:18 AM Kieran Bingham <
> kieran.bingham at ideasonboard.com> wrote:
> 
> > Quoting Nicholas Roth (2022-10-27 05:24:28)
> > > >  cp utils/hooks/post-commit .git/hooks/post-commit
> > > The post-commits seem to want me to replace my tabs with spaces. I use
> > tabs
> > > to be consistent with the surrounding codebase. Is it OK to ignore these
> > > errors, or should I do something else?
> >
> > That's odd - it shouldn't be changing tabs for spaces?
> >
> > What's your tabsize set to ? We use 8-character tabs.
> >
> >
> > > Example:
> > > --- src/android/camera_hal_config.cpp
> > > +++ src/android/camera_hal_config.cpp
> > > @@ -164,7 +164,7 @@
> > >         File file(filePath);
> > >         if (!file.exist()) {
> > >                 LOG(HALConfig, Debug)
> > > -                                       << "Configuration file: \"" <<
> > > filePath << "\" not found";
> > > +                       << "Configuration file: \"" << filePath << "\"
> > not
> > > found";
> > >                 return -ENOENT;
> >
> > In my terminal, this change looks correct:
> >
> >                 LOG(HALConfig, Debug)
> >                         << "Configuration file: \"" << filePath << "\" not
> > found";
> >
> > We often take the debug print lines to the following line to allow a
> > longer length without breaking the text, or hitting our line length
> > limit. We only use a single indentation from the start of the previous
> > line
> >
> > --
> > Kieran
> >
> >
> > >         }
> > >
> > > ---
> > > 1 potential issue detected, please review
> >