| Message ID | 20240830152721.1420313-1-mzamazal@redhat.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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(-)
Hi Laurent, Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > 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 ? Yes. > 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 :-) Thank you. >> 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. OK. For now, I simply reverted the unwanted changes and kept those that you didn't object strongly enough. We can work on improvements later, as needed. >> 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(-)
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, - 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 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 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). 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. 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. 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(-)