[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