[PATCH v2 00/20] Remove unused includes

Laurent Pinchart laurent.pinchart at ideasonboard.com
Sat Aug 31 02:52:01 CEST 2024


Hi Milan,

Thank you for the series. It looks like you went through a rabbit hole
:-)

On Fri, Aug 30, 2024 at 05:26:57PM +0200, Milan Zamazal wrote:
> There is quite a lot of include’s that are actually unused.  They don’t
> cause much trouble but they should still be removed in order to:
> 
> - keep order,
> - avoid LSP warnings when working with the code,

Which LSP server do you use ? clangd ? I assume it's based on
clang-format for coding formatting, so that should be fine as that's we
use in checkstyle.py too.

> - to remove unneeded extra lines.
> 
> I have identified the unused includes by inspecting warnings issued by
> LSP.  LSP understands the difference between direct and indirect
> imports.  I have inspected its reports in some cases and it seems to be
> correct most of the time so I trust it and haven’t inspected everything,
> which would be waste of resources.

I wasted some resources, sorry :-)

> I have noticed only two problems:
> 
> - LSP reports in my environment unused <chrono> includes despite of
>   presence of
> 
>     using std::literals::chrono_literals;
> 
>   or
> 
>     using std::chrono_literals;
> 
>   (these two should be probably unified but this is out of scope of this
>   series).  This happens also in a trivial code and is most likely a
>   bug.
> 
> - LSP may report unnecessary inclusion of
>   "libcamera/internal/framebuffer.h" although it is actually needed to
>   be able to access something from FrameBuffer::Private.
> 
> It’s interesting that in both the cases LSP reports the corresponding
> problems when the supposedly unused include is removed.
> 
> I’m also not sure whether header include removal can affect Doxygen
> output in some way.  I supposed it doesn’t but if it does then the
> patches must be inspected for such cases.

I'm not aware of any issue there.

> I haven’t removed includes of "FOO.h" in FOO.cpp files even if the
> include is unused (which happens when FOO.cpp contains only
> documentation).

Good choice. Include the .h helps making sure it's self-contained.

> My LSP based autoformatter reformatted some of the modified files.  Many
> of the suggested changes look right so I kept most of them (and helped
> the autoformatter to do a better thing in some cases), in separate
> commits, rather than discarding them.  It’s annoying when editing a
> source file and the autoformatter changes the code elsewhere; if the
> autoformatter is wrong anywhere then the autoformatting rules should be
> fixed if possible.

Some of the changes look right and good, some are more in neutral
territory to me, but quite a few I don't like. Last time I looked at
clang-format there I couldn't get it to match our coding style
perfectly, but new options may have been added. If so, I agree it would
be best to bring it as close as possible to how we want code to be
formatted. There will always be exceptions though.

> I have inspected many source files but not all of them.  Although the
> cleanup is most likely a bit incomplete, it should still make things
> significantly better.
> 
> I’m sorry for another long patch series but I tried to split the patches
> in order to make them reasonably sized (and easy to review!) and by
> possible areas of responsibility.

You did well, thank you for that.

> Changes in v2:
> - Add a missing include in tests.
> 
> Milan Zamazal (20):
>   tests: Add a missing iostream include
>   libcamera: ipu3: Remove unused includes
>   libcamera: ipu3: Replace wrong include
>   libcamera: ipu3: Formatting improvements
>   libcamera: rkisp1: Remove unused includes
>   libcamera: rkisp1: Formatting improvements
>   libcamera: libipa: Remove unused includes
>   libcamera: uvcvideo: Remove unused includes
>   libcamera: uvcvideo: Formatting improvement
>   libcamera: v4l2: Remove unused includes
>   libcamera: v4l2: Formatting improvements
>   libcamera: v4l2_subdevice: Formatting improvements
>   libcamera: v4l2: Fix indirect include
>   libcamera: ipa: Remove unused includes
>   libcamera: libcamera: Remove unused includes
>   libcamera: libcamera: Add missing includes
>   libcamera: libcamera: Formatting improvements
>   libcamera: includes: Add missing includes
>   libcamera: includes: Remove unused includes
>   libcamera: includes: Formatting improvements
> 
>  include/libcamera/base/event_dispatcher.h     |    2 -
>  include/libcamera/base/log.h                  |   23 +-
>  include/libcamera/base/memfd.h                |    2 -
>  include/libcamera/base/signal.h               |    1 -
>  include/libcamera/base/span.h                 |   41 +-
>  include/libcamera/base/timer.h                |    1 -
>  include/libcamera/base/utils.h                |    3 +-
>  include/libcamera/framebuffer.h               |    1 -
>  include/libcamera/internal/camera_manager.h   |    5 +-
>  include/libcamera/internal/camera_sensor.h    |    1 -
>  .../internal/device_enumerator_sysfs.h        |    1 -
>  .../libcamera/internal/dma_buf_allocator.h    |    2 -
>  include/libcamera/internal/formats.h          |    1 -
>  .../libcamera/internal/ipa_data_serializer.h  |   11 +-
>  include/libcamera/internal/ipa_proxy.h        |    2 -
>  .../libcamera/internal/ipc_pipe_unixsocket.h  |    1 -
>  include/libcamera/internal/media_device.h     |    1 -
>  include/libcamera/internal/pipeline_handler.h |    3 -
>  include/libcamera/internal/request.h          |    1 +
>  .../libcamera/internal/shared_mem_object.h    |    1 -
>  include/libcamera/ipa/ipa_interface.h         |    5 -
>  include/libcamera/logging.h                   |    2 +
>  include/libcamera/pixel_format.h              |    1 -
>  include/libcamera/request.h                   |    1 -
>  include/libcamera/stream.h                    |    1 -
>  include/libcamera/transform.h                 |    2 -
>  src/ipa/ipu3/algorithms/af.cpp                |    3 -
>  src/ipa/ipu3/algorithms/agc.cpp               |   32 +-
>  src/ipa/ipu3/algorithms/blc.cpp               |    6 +-
>  src/ipa/ipu3/ipu3.cpp                         |   16 +-
>  src/ipa/libipa/histogram.h                    |    1 -
>  src/ipa/libipa/matrix.h                       |    1 -
>  src/ipa/libipa/matrix_interpolator.cpp        |    7 -
>  src/ipa/libipa/matrix_interpolator.h          |    2 -
>  src/ipa/libipa/pwl.cpp                        |    2 -
>  src/ipa/libipa/pwl.h                          |    3 -
>  src/ipa/libipa/vector.h                       |    2 -
>  src/ipa/rkisp1/algorithms/agc.h               |    1 -
>  src/ipa/rkisp1/algorithms/awb.cpp             |   20 +-
>  src/ipa/rkisp1/algorithms/ccm.cpp             |    4 -
>  src/ipa/rkisp1/algorithms/dpf.cpp             |    6 +-
>  src/ipa/rkisp1/rkisp1.cpp                     |   22 +-
>  src/ipa/rkisp1/utils.h                        |    1 -
>  src/libcamera/base/event_dispatcher_poll.cpp  |    4 +-
>  src/libcamera/camera.cpp                      |    6 +-
>  src/libcamera/controls.cpp                    |   32 +-
>  .../converter/converter_v4l2_m2m.cpp          |    1 -
>  src/libcamera/formats.cpp                     |    3 +-
>  src/libcamera/ipa_data_serializer.cpp         |   97 +-
>  src/libcamera/ipa_module.cpp                  |   16 +-
>  src/libcamera/ipa_proxy.cpp                   |    1 -
>  src/libcamera/orientation.cpp                 |   17 +-
>  src/libcamera/pipeline/ipu3/frames.cpp        |    3 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |   41 +-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |    2 -
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |    6 +-
>  src/libcamera/pipeline_handler.cpp            |    6 +-
>  src/libcamera/process.cpp                     |    8 +-
>  src/libcamera/sensor/camera_sensor.cpp        |    8 +-
>  src/libcamera/shared_mem_object.cpp           |    5 +-
>  src/libcamera/stream.cpp                      |    8 +-
>  src/libcamera/v4l2_device.cpp                 |   19 +-
>  src/libcamera/v4l2_subdevice.cpp              | 1311 +++++++++--------
>  src/libcamera/v4l2_videodevice.cpp            |   37 +-
>  src/v4l2/v4l2_camera.h                        |    1 -
>  src/v4l2/v4l2_camera_proxy.cpp                |   46 +-
>  src/v4l2/v4l2_compat.cpp                      |   20 +-
>  src/v4l2/v4l2_compat_manager.cpp              |    5 +-
>  .../generated_serializer_test.cpp             |    1 +
>  69 files changed, 967 insertions(+), 982 deletions(-)

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list