| Message ID | 20211123224015.3619282-1-kieran.bingham@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch series. On Tue, Nov 23, 2021 at 10:40:00PM +0000, Kieran Bingham wrote: > We've always used the following pattern to maintain idempotency with > headers: > > #ifndef __PATH_TO_FILE_H__ > #define __PATH_TO_FILE_H__ > .. > #endif // __PATH_TO_FILE_H__ Turns out we had only two mismatches in header guards where the comment after the #endif didn't match the macro name, not too bad :-) > This is fine, and long established, but makes for awkward names in cases > such as: > src/ipa/ipu3/algorithm/algorithm.h > > requiring duplication, and constant concern over updating these names > when files are moved or renamed. > > Remove all ifndef usages and ensure consistency across the project using > > #pragma once > > The changes are broken down into grouped components to ease review. Thanks, that makes it easier to review the series. I have one comment for patch 03/15. For the rest of the series, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Kieran Bingham (15): > libcamera: Convert to pragma once > libcamera: base: Convert to pragma once > libcamera: internal: Convert to pragma once > libcamera: ipa: Convert to pragma once > libcamera: pipeline: Convert to pragma once > android: Convert to pragma once > cam: Convert to pragma once > gstreamer: Convert to pragma once > ipa: ipu3: Convert to pragma once > ipa: libipa: Convert to pragma once > lc-compliance: Convert to pragma once > qcam: Convert to pragma once > test: Convert to pragma once > utils: Convert to pragma once > v4l2: Convert to pragma once > > include/libcamera/base/backtrace.h | 6 ++---- > include/libcamera/base/bound_method.h | 6 ++---- > include/libcamera/base/class.h | 6 ++---- > include/libcamera/base/event_dispatcher.h | 6 ++---- > include/libcamera/base/event_dispatcher_poll.h | 6 ++---- > include/libcamera/base/event_notifier.h | 6 ++---- > include/libcamera/base/file.h | 6 ++---- > include/libcamera/base/flags.h | 6 ++---- > include/libcamera/base/log.h | 6 ++---- > include/libcamera/base/message.h | 6 ++---- > include/libcamera/base/object.h | 6 ++---- > include/libcamera/base/semaphore.h | 6 ++---- > include/libcamera/base/signal.h | 6 ++---- > include/libcamera/base/span.h | 5 +---- > include/libcamera/base/thread.h | 6 ++---- > include/libcamera/base/timer.h | 6 ++---- > include/libcamera/base/utils.h | 6 ++---- > include/libcamera/camera.h | 6 ++---- > include/libcamera/camera_manager.h | 6 ++---- > include/libcamera/compiler.h | 6 ++---- > include/libcamera/control_ids.h.in | 5 +---- > include/libcamera/controls.h | 5 +---- > include/libcamera/file_descriptor.h | 6 ++---- > include/libcamera/formats.h.in | 6 ++---- > include/libcamera/framebuffer.h | 6 ++---- > include/libcamera/framebuffer_allocator.h | 6 ++---- > include/libcamera/geometry.h | 5 +---- > include/libcamera/internal/bayer_format.h | 6 ++---- > include/libcamera/internal/byte_stream_buffer.h | 6 ++---- > include/libcamera/internal/camera.h | 6 ++---- > include/libcamera/internal/camera_controls.h | 6 ++---- > include/libcamera/internal/camera_sensor.h | 6 ++---- > include/libcamera/internal/camera_sensor_properties.h | 6 ++---- > include/libcamera/internal/control_serializer.h | 6 ++---- > include/libcamera/internal/control_validator.h | 6 ++---- > include/libcamera/internal/delayed_controls.h | 6 ++---- > include/libcamera/internal/device_enumerator.h | 6 ++---- > include/libcamera/internal/device_enumerator_sysfs.h | 6 ++---- > include/libcamera/internal/device_enumerator_udev.h | 6 ++---- > include/libcamera/internal/formats.h | 5 +---- > include/libcamera/internal/framebuffer.h | 6 ++---- > include/libcamera/internal/ipa_data_serializer.h | 6 ++---- > include/libcamera/internal/ipa_manager.h | 6 ++---- > include/libcamera/internal/ipa_module.h | 6 ++---- > include/libcamera/internal/ipa_proxy.h | 6 ++---- > include/libcamera/internal/ipc_pipe.h | 6 ++---- > include/libcamera/internal/ipc_pipe_unixsocket.h | 6 ++---- > include/libcamera/internal/ipc_unixsocket.h | 5 +---- > include/libcamera/internal/mapped_framebuffer.h | 6 ++---- > include/libcamera/internal/media_device.h | 6 ++---- > include/libcamera/internal/media_object.h | 6 ++---- > include/libcamera/internal/pipeline_handler.h | 6 ++---- > include/libcamera/internal/process.h | 6 ++---- > include/libcamera/internal/pub_key.h | 6 ++---- > include/libcamera/internal/source_paths.h | 6 ++---- > include/libcamera/internal/sysfs.h | 6 ++---- > include/libcamera/internal/tracepoints.h.in | 6 ++---- > include/libcamera/internal/v4l2_device.h | 6 ++---- > include/libcamera/internal/v4l2_pixelformat.h | 6 ++---- > include/libcamera/internal/v4l2_subdevice.h | 6 ++---- > include/libcamera/internal/v4l2_videodevice.h | 6 ++---- > include/libcamera/ipa/ipa_controls.h | 6 ++---- > include/libcamera/ipa/ipa_interface.h | 6 ++---- > include/libcamera/ipa/ipa_module_info.h | 6 ++---- > include/libcamera/ipa/raspberrypi.h | 6 ++---- > include/libcamera/logging.h | 6 ++---- > include/libcamera/pixel_format.h | 6 ++---- > include/libcamera/property_ids.h.in | 5 +---- > include/libcamera/request.h | 6 ++---- > include/libcamera/stream.h | 6 ++---- > include/libcamera/transform.h | 5 +---- > include/libcamera/version.h.in | 6 ++---- > src/android/camera_buffer.h | 5 ++--- > src/android/camera_capabilities.h | 6 ++---- > src/android/camera_device.h | 6 ++---- > src/android/camera_hal_config.h | 5 ++--- > src/android/camera_hal_manager.h | 6 ++---- > src/android/camera_metadata.h | 6 ++---- > src/android/camera_ops.h | 6 ++---- > src/android/camera_request.h | 6 ++---- > src/android/camera_stream.h | 6 ++---- > src/android/camera_worker.h | 6 ++---- > src/android/jpeg/encoder.h | 6 ++---- > src/android/jpeg/encoder_libjpeg.h | 6 ++---- > src/android/jpeg/exif.h | 6 ++---- > src/android/jpeg/post_processor_jpeg.h | 6 ++---- > src/android/jpeg/thumbnailer.h | 6 ++---- > src/android/post_processor.h | 6 ++---- > src/android/yuv/post_processor_yuv.h | 6 ++---- > src/cam/camera_session.h | 6 ++---- > src/cam/drm.h | 6 ++---- > src/cam/event_loop.h | 6 ++---- > src/cam/file_sink.h | 6 ++---- > src/cam/frame_sink.h | 6 ++---- > src/cam/image.h | 6 ++---- > src/cam/kms_sink.h | 6 ++---- > src/cam/main.h | 6 ++---- > src/cam/options.h | 6 ++---- > src/cam/stream_options.h | 6 ++---- > src/gstreamer/gstlibcamera-utils.h | 5 +---- > src/gstreamer/gstlibcameraallocator.h | 5 +---- > src/gstreamer/gstlibcamerapad.h | 5 +---- > src/gstreamer/gstlibcamerapool.h | 6 +----- > src/gstreamer/gstlibcameraprovider.h | 6 +----- > src/gstreamer/gstlibcamerasrc.h | 5 +---- > src/ipa/ipu3/algorithms/agc.h | 6 ++---- > src/ipa/ipu3/algorithms/algorithm.h | 6 ++---- > src/ipa/ipu3/algorithms/awb.h | 5 ++--- > src/ipa/ipu3/algorithms/blc.h | 6 ++---- > src/ipa/ipu3/algorithms/tone_mapping.h | 6 ++---- > src/ipa/ipu3/ipa_context.h | 6 ++---- > src/ipa/libipa/camera_sensor_helper.h | 6 ++---- > src/ipa/libipa/histogram.h | 6 ++---- > src/lc-compliance/environment.h | 6 ++---- > src/lc-compliance/simple_capture.h | 6 ++---- > src/libcamera/pipeline/ipu3/cio2.h | 6 ++---- > src/libcamera/pipeline/ipu3/frames.h | 6 ++---- > src/libcamera/pipeline/ipu3/imgu.h | 6 ++---- > src/libcamera/pipeline/raspberrypi/dma_heaps.h | 6 ++---- > src/libcamera/pipeline/raspberrypi/rpi_stream.h | 6 ++---- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 ++---- > src/libcamera/pipeline/simple/converter.h | 5 +---- > src/qcam/dng_writer.h | 6 ++---- > src/qcam/format_converter.h | 6 ++---- > src/qcam/main_window.h | 6 ++---- > src/qcam/message_handler.h | 6 ++---- > src/qcam/viewfinder.h | 6 ++---- > src/qcam/viewfinder_gl.h | 6 ++---- > src/qcam/viewfinder_qt.h | 6 ++---- > src/v4l2/v4l2_camera.h | 5 +---- > src/v4l2/v4l2_camera_file.h | 5 +---- > src/v4l2/v4l2_camera_proxy.h | 5 +---- > src/v4l2/v4l2_compat_manager.h | 5 +---- > test/gstreamer/gstreamer_test.h | 5 +---- > test/libtest/buffer_source.h | 6 ++---- > test/libtest/camera_test.h | 6 ++---- > test/libtest/test.h | 6 ++---- > test/media_device/media_device_test.h | 6 ++---- > test/serialization/serialization_test.h | 6 ++---- > test/v4l2_subdevice/v4l2_subdevice_test.h | 5 +---- > test/v4l2_videodevice/v4l2_videodevice_test.h | 6 ++---- > utils/gen-header.sh | 9 ++------- > .../libcamera_templates/core_ipa_interface.h.tmpl | 5 +---- > .../libcamera_templates/core_ipa_serializer.h.tmpl | 5 +---- > .../libcamera_templates/module_ipa_interface.h.tmpl | 5 +---- > .../libcamera_templates/module_ipa_proxy.h.tmpl | 5 +---- > .../libcamera_templates/module_ipa_serializer.h.tmpl | 5 +---- > 147 files changed, 268 insertions(+), 590 deletions(-)
Hi Kieran, Thanks for the (very quickly posted) series ! On 24/11/2021 04:00, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch series. > > On Tue, Nov 23, 2021 at 10:40:00PM +0000, Kieran Bingham wrote: >> We've always used the following pattern to maintain idempotency with >> headers: >> >> #ifndef __PATH_TO_FILE_H__ >> #define __PATH_TO_FILE_H__ >> .. >> #endif // __PATH_TO_FILE_H__ > > Turns out we had only two mismatches in header guards where the comment > after the #endif didn't match the macro name, not too bad :-) > >> This is fine, and long established, but makes for awkward names in cases >> such as: >> src/ipa/ipu3/algorithm/algorithm.h >> >> requiring duplication, and constant concern over updating these names >> when files are moved or renamed. >> >> Remove all ifndef usages and ensure consistency across the project using >> >> #pragma once >> >> The changes are broken down into grouped components to ease review. > > Thanks, that makes it easier to review the series. > > I have one comment for patch 03/15. For the rest of the series, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Kieran Bingham (15): >> libcamera: Convert to pragma once >> libcamera: base: Convert to pragma once >> libcamera: internal: Convert to pragma once >> libcamera: ipa: Convert to pragma once >> libcamera: pipeline: Convert to pragma once >> android: Convert to pragma once >> cam: Convert to pragma once >> gstreamer: Convert to pragma once >> ipa: ipu3: Convert to pragma once >> ipa: libipa: Convert to pragma once >> lc-compliance: Convert to pragma once >> qcam: Convert to pragma once >> test: Convert to pragma once >> utils: Convert to pragma once >> v4l2: Convert to pragma once >> >> include/libcamera/base/backtrace.h | 6 ++---- >> include/libcamera/base/bound_method.h | 6 ++---- >> include/libcamera/base/class.h | 6 ++---- >> include/libcamera/base/event_dispatcher.h | 6 ++---- >> include/libcamera/base/event_dispatcher_poll.h | 6 ++---- >> include/libcamera/base/event_notifier.h | 6 ++---- >> include/libcamera/base/file.h | 6 ++---- >> include/libcamera/base/flags.h | 6 ++---- >> include/libcamera/base/log.h | 6 ++---- >> include/libcamera/base/message.h | 6 ++---- >> include/libcamera/base/object.h | 6 ++---- >> include/libcamera/base/semaphore.h | 6 ++---- >> include/libcamera/base/signal.h | 6 ++---- >> include/libcamera/base/span.h | 5 +---- >> include/libcamera/base/thread.h | 6 ++---- >> include/libcamera/base/timer.h | 6 ++---- >> include/libcamera/base/utils.h | 6 ++---- >> include/libcamera/camera.h | 6 ++---- >> include/libcamera/camera_manager.h | 6 ++---- >> include/libcamera/compiler.h | 6 ++---- >> include/libcamera/control_ids.h.in | 5 +---- >> include/libcamera/controls.h | 5 +---- >> include/libcamera/file_descriptor.h | 6 ++---- >> include/libcamera/formats.h.in | 6 ++---- >> include/libcamera/framebuffer.h | 6 ++---- >> include/libcamera/framebuffer_allocator.h | 6 ++---- >> include/libcamera/geometry.h | 5 +---- >> include/libcamera/internal/bayer_format.h | 6 ++---- >> include/libcamera/internal/byte_stream_buffer.h | 6 ++---- >> include/libcamera/internal/camera.h | 6 ++---- >> include/libcamera/internal/camera_controls.h | 6 ++---- >> include/libcamera/internal/camera_sensor.h | 6 ++---- >> include/libcamera/internal/camera_sensor_properties.h | 6 ++---- >> include/libcamera/internal/control_serializer.h | 6 ++---- >> include/libcamera/internal/control_validator.h | 6 ++---- >> include/libcamera/internal/delayed_controls.h | 6 ++---- >> include/libcamera/internal/device_enumerator.h | 6 ++---- >> include/libcamera/internal/device_enumerator_sysfs.h | 6 ++---- >> include/libcamera/internal/device_enumerator_udev.h | 6 ++---- >> include/libcamera/internal/formats.h | 5 +---- >> include/libcamera/internal/framebuffer.h | 6 ++---- >> include/libcamera/internal/ipa_data_serializer.h | 6 ++---- >> include/libcamera/internal/ipa_manager.h | 6 ++---- >> include/libcamera/internal/ipa_module.h | 6 ++---- >> include/libcamera/internal/ipa_proxy.h | 6 ++---- >> include/libcamera/internal/ipc_pipe.h | 6 ++---- >> include/libcamera/internal/ipc_pipe_unixsocket.h | 6 ++---- >> include/libcamera/internal/ipc_unixsocket.h | 5 +---- >> include/libcamera/internal/mapped_framebuffer.h | 6 ++---- >> include/libcamera/internal/media_device.h | 6 ++---- >> include/libcamera/internal/media_object.h | 6 ++---- >> include/libcamera/internal/pipeline_handler.h | 6 ++---- >> include/libcamera/internal/process.h | 6 ++---- >> include/libcamera/internal/pub_key.h | 6 ++---- >> include/libcamera/internal/source_paths.h | 6 ++---- >> include/libcamera/internal/sysfs.h | 6 ++---- >> include/libcamera/internal/tracepoints.h.in | 6 ++---- >> include/libcamera/internal/v4l2_device.h | 6 ++---- >> include/libcamera/internal/v4l2_pixelformat.h | 6 ++---- >> include/libcamera/internal/v4l2_subdevice.h | 6 ++---- >> include/libcamera/internal/v4l2_videodevice.h | 6 ++---- >> include/libcamera/ipa/ipa_controls.h | 6 ++---- >> include/libcamera/ipa/ipa_interface.h | 6 ++---- >> include/libcamera/ipa/ipa_module_info.h | 6 ++---- >> include/libcamera/ipa/raspberrypi.h | 6 ++---- >> include/libcamera/logging.h | 6 ++---- >> include/libcamera/pixel_format.h | 6 ++---- >> include/libcamera/property_ids.h.in | 5 +---- >> include/libcamera/request.h | 6 ++---- >> include/libcamera/stream.h | 6 ++---- >> include/libcamera/transform.h | 5 +---- >> include/libcamera/version.h.in | 6 ++---- >> src/android/camera_buffer.h | 5 ++--- >> src/android/camera_capabilities.h | 6 ++---- >> src/android/camera_device.h | 6 ++---- >> src/android/camera_hal_config.h | 5 ++--- >> src/android/camera_hal_manager.h | 6 ++---- >> src/android/camera_metadata.h | 6 ++---- >> src/android/camera_ops.h | 6 ++---- >> src/android/camera_request.h | 6 ++---- >> src/android/camera_stream.h | 6 ++---- >> src/android/camera_worker.h | 6 ++---- >> src/android/jpeg/encoder.h | 6 ++---- >> src/android/jpeg/encoder_libjpeg.h | 6 ++---- >> src/android/jpeg/exif.h | 6 ++---- >> src/android/jpeg/post_processor_jpeg.h | 6 ++---- >> src/android/jpeg/thumbnailer.h | 6 ++---- >> src/android/post_processor.h | 6 ++---- >> src/android/yuv/post_processor_yuv.h | 6 ++---- >> src/cam/camera_session.h | 6 ++---- >> src/cam/drm.h | 6 ++---- >> src/cam/event_loop.h | 6 ++---- >> src/cam/file_sink.h | 6 ++---- >> src/cam/frame_sink.h | 6 ++---- >> src/cam/image.h | 6 ++---- >> src/cam/kms_sink.h | 6 ++---- >> src/cam/main.h | 6 ++---- >> src/cam/options.h | 6 ++---- >> src/cam/stream_options.h | 6 ++---- >> src/gstreamer/gstlibcamera-utils.h | 5 +---- >> src/gstreamer/gstlibcameraallocator.h | 5 +---- >> src/gstreamer/gstlibcamerapad.h | 5 +---- >> src/gstreamer/gstlibcamerapool.h | 6 +----- >> src/gstreamer/gstlibcameraprovider.h | 6 +----- >> src/gstreamer/gstlibcamerasrc.h | 5 +---- >> src/ipa/ipu3/algorithms/agc.h | 6 ++---- >> src/ipa/ipu3/algorithms/algorithm.h | 6 ++---- >> src/ipa/ipu3/algorithms/awb.h | 5 ++--- >> src/ipa/ipu3/algorithms/blc.h | 6 ++---- >> src/ipa/ipu3/algorithms/tone_mapping.h | 6 ++---- >> src/ipa/ipu3/ipa_context.h | 6 ++---- >> src/ipa/libipa/camera_sensor_helper.h | 6 ++---- >> src/ipa/libipa/histogram.h | 6 ++---- I need to use this #pragma once in v4 for the RkISP1 then :-). For the series: Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> src/lc-compliance/environment.h | 6 ++---- >> src/lc-compliance/simple_capture.h | 6 ++---- >> src/libcamera/pipeline/ipu3/cio2.h | 6 ++---- >> src/libcamera/pipeline/ipu3/frames.h | 6 ++---- >> src/libcamera/pipeline/ipu3/imgu.h | 6 ++---- >> src/libcamera/pipeline/raspberrypi/dma_heaps.h | 6 ++---- >> src/libcamera/pipeline/raspberrypi/rpi_stream.h | 6 ++---- >> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 ++---- >> src/libcamera/pipeline/simple/converter.h | 5 +---- >> src/qcam/dng_writer.h | 6 ++---- >> src/qcam/format_converter.h | 6 ++---- >> src/qcam/main_window.h | 6 ++---- >> src/qcam/message_handler.h | 6 ++---- >> src/qcam/viewfinder.h | 6 ++---- >> src/qcam/viewfinder_gl.h | 6 ++---- >> src/qcam/viewfinder_qt.h | 6 ++---- >> src/v4l2/v4l2_camera.h | 5 +---- >> src/v4l2/v4l2_camera_file.h | 5 +---- >> src/v4l2/v4l2_camera_proxy.h | 5 +---- >> src/v4l2/v4l2_compat_manager.h | 5 +---- >> test/gstreamer/gstreamer_test.h | 5 +---- >> test/libtest/buffer_source.h | 6 ++---- >> test/libtest/camera_test.h | 6 ++---- >> test/libtest/test.h | 6 ++---- >> test/media_device/media_device_test.h | 6 ++---- >> test/serialization/serialization_test.h | 6 ++---- >> test/v4l2_subdevice/v4l2_subdevice_test.h | 5 +---- >> test/v4l2_videodevice/v4l2_videodevice_test.h | 6 ++---- >> utils/gen-header.sh | 9 ++------- >> .../libcamera_templates/core_ipa_interface.h.tmpl | 5 +---- >> .../libcamera_templates/core_ipa_serializer.h.tmpl | 5 +---- >> .../libcamera_templates/module_ipa_interface.h.tmpl | 5 +---- >> .../libcamera_templates/module_ipa_proxy.h.tmpl | 5 +---- >> .../libcamera_templates/module_ipa_serializer.h.tmpl | 5 +---- >> 147 files changed, 268 insertions(+), 590 deletions(-) >