| Message ID | 20211005073114.3997303-1-hiroh@chromium.org |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On Tue, Oct 05, 2021 at 04:31:08PM +0900, Hirokazu Honda wrote: > According to Google C++ style guide, using-directives (i.e. > "using namespace foo") should not be used. However, libcamera has > been using the scheme in many files. Removing using-directives > entirely in the code definitely needs the dicussion. Indeed, that would need discussions :-) Part of the discussion would likely touch the namespace name, libcamera is fairly long. > This patch series resolves the porblem of using using-directives > in header file, which I expect I can handily get the agreement > from developers. Using-directives in a header file is propagated > to all the files including the header file. This pollutes > namespace severely. Overall it looks fine to me, although I'm not sure it's such a big deal. Yes, using directives are propagated, which is a bad idea in general, but the header files below are all internal the the component they relate to. The risk of namespace polution is thus more theoretical than practical. This being said, we're already using this policy in the HAL headers, and I don't object extending it to all headers. I'll side with the majority. > [1] https://google.github.io/styleguide/cppguide.html#Namespaces > > Hirokazu Honda (6): > ipa: raspberrypi: agc: Remove using namespace in agc.hpp > cam: Remove using namespace in stream_options.h > qcam: Remove using namespace in header files > v4l2: Remove using namespace in header files > lc-compliance: Remove using namespace in header files > test: Remove using namespace in header files > > src/cam/stream_options.h | 8 ++-- > src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 ++++ > src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +-- > src/lc-compliance/environment.cpp | 2 +- > src/lc-compliance/environment.h | 8 ++-- > src/qcam/dng_writer.h | 10 ++--- > src/qcam/main.cpp | 2 +- > src/qcam/main_window.h | 40 +++++++++---------- > src/v4l2/v4l2_camera.h | 37 +++++++++-------- > src/v4l2/v4l2_camera_proxy.h | 15 ++++--- > src/v4l2/v4l2_compat_manager.h | 4 +- > test/camera/camera_reconfigure.cpp | 1 + > test/camera/capture.cpp | 1 + > test/camera/configuration_default.cpp | 1 + > test/camera/configuration_set.cpp | 1 + > test/camera/statemachine.cpp | 1 + > test/gstreamer/gstreamer_test.h | 2 - > test/libtest/buffer_source.cpp | 2 + > test/libtest/buffer_source.h | 10 ++--- > test/libtest/camera_test.h | 6 +-- > test/mapped-buffer.cpp | 1 + > test/media_device/media_device_test.h | 6 +-- > test/serialization/serialization_test.h | 10 ++--- > test/v4l2_subdevice/v4l2_subdevice_test.h | 8 ++-- > test/v4l2_videodevice/buffer_sharing.cpp | 2 + > test/v4l2_videodevice/capture_async.cpp | 2 + > test/v4l2_videodevice/v4l2_videodevice_test.h | 14 +++---- > 27 files changed, 100 insertions(+), 107 deletions(-)
Hi Laurent, Would you merge this patch series? On Tue, Oct 5, 2021 at 6:51 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Tue, Oct 05, 2021 at 04:31:08PM +0900, Hirokazu Honda wrote: > > According to Google C++ style guide, using-directives (i.e. > > "using namespace foo") should not be used. However, libcamera has > > been using the scheme in many files. Removing using-directives > > entirely in the code definitely needs the dicussion. > > Indeed, that would need discussions :-) Part of the discussion would > likely touch the namespace name, libcamera is fairly long. > > > This patch series resolves the porblem of using using-directives > > in header file, which I expect I can handily get the agreement > > from developers. Using-directives in a header file is propagated > > to all the files including the header file. This pollutes > > namespace severely. > > Overall it looks fine to me, although I'm not sure it's such a big deal. > Yes, using directives are propagated, which is a bad idea in general, > but the header files below are all internal the the component they > relate to. The risk of namespace polution is thus more theoretical than > practical. > > This being said, we're already using this policy in the HAL headers, and > I don't object extending it to all headers. I'll side with the majority. > > > [1] https://google.github.io/styleguide/cppguide.html#Namespaces > > > > Hirokazu Honda (6): > > ipa: raspberrypi: agc: Remove using namespace in agc.hpp > > cam: Remove using namespace in stream_options.h > > qcam: Remove using namespace in header files > > v4l2: Remove using namespace in header files > > lc-compliance: Remove using namespace in header files > > test: Remove using namespace in header files > > > > src/cam/stream_options.h | 8 ++-- > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 ++++ > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +-- > > src/lc-compliance/environment.cpp | 2 +- > > src/lc-compliance/environment.h | 8 ++-- > > src/qcam/dng_writer.h | 10 ++--- > > src/qcam/main.cpp | 2 +- > > src/qcam/main_window.h | 40 +++++++++---------- > > src/v4l2/v4l2_camera.h | 37 +++++++++-------- > > src/v4l2/v4l2_camera_proxy.h | 15 ++++--- > > src/v4l2/v4l2_compat_manager.h | 4 +- > > test/camera/camera_reconfigure.cpp | 1 + > > test/camera/capture.cpp | 1 + > > test/camera/configuration_default.cpp | 1 + > > test/camera/configuration_set.cpp | 1 + > > test/camera/statemachine.cpp | 1 + > > test/gstreamer/gstreamer_test.h | 2 - > > test/libtest/buffer_source.cpp | 2 + > > test/libtest/buffer_source.h | 10 ++--- > > test/libtest/camera_test.h | 6 +-- > > test/mapped-buffer.cpp | 1 + > > test/media_device/media_device_test.h | 6 +-- > > test/serialization/serialization_test.h | 10 ++--- > > test/v4l2_subdevice/v4l2_subdevice_test.h | 8 ++-- > > test/v4l2_videodevice/buffer_sharing.cpp | 2 + > > test/v4l2_videodevice/capture_async.cpp | 2 + > > test/v4l2_videodevice/v4l2_videodevice_test.h | 14 +++---- > > 27 files changed, 100 insertions(+), 107 deletions(-) > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Wed, Oct 20, 2021 at 11:26:26AM +0900, Hirokazu Honda wrote: > Hi Laurent, > > Would you merge this patch series? It has been merged already :-) > On Tue, Oct 5, 2021 at 6:51 PM Laurent Pinchart wrote: > > On Tue, Oct 05, 2021 at 04:31:08PM +0900, Hirokazu Honda wrote: > > > According to Google C++ style guide, using-directives (i.e. > > > "using namespace foo") should not be used. However, libcamera has > > > been using the scheme in many files. Removing using-directives > > > entirely in the code definitely needs the dicussion. > > > > Indeed, that would need discussions :-) Part of the discussion would > > likely touch the namespace name, libcamera is fairly long. > > > > > This patch series resolves the porblem of using using-directives > > > in header file, which I expect I can handily get the agreement > > > from developers. Using-directives in a header file is propagated > > > to all the files including the header file. This pollutes > > > namespace severely. > > > > Overall it looks fine to me, although I'm not sure it's such a big deal. > > Yes, using directives are propagated, which is a bad idea in general, > > but the header files below are all internal the the component they > > relate to. The risk of namespace polution is thus more theoretical than > > practical. > > > > This being said, we're already using this policy in the HAL headers, and > > I don't object extending it to all headers. I'll side with the majority. > > > > > [1] https://google.github.io/styleguide/cppguide.html#Namespaces > > > > > > Hirokazu Honda (6): > > > ipa: raspberrypi: agc: Remove using namespace in agc.hpp > > > cam: Remove using namespace in stream_options.h > > > qcam: Remove using namespace in header files > > > v4l2: Remove using namespace in header files > > > lc-compliance: Remove using namespace in header files > > > test: Remove using namespace in header files > > > > > > src/cam/stream_options.h | 8 ++-- > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 ++++ > > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +-- > > > src/lc-compliance/environment.cpp | 2 +- > > > src/lc-compliance/environment.h | 8 ++-- > > > src/qcam/dng_writer.h | 10 ++--- > > > src/qcam/main.cpp | 2 +- > > > src/qcam/main_window.h | 40 +++++++++---------- > > > src/v4l2/v4l2_camera.h | 37 +++++++++-------- > > > src/v4l2/v4l2_camera_proxy.h | 15 ++++--- > > > src/v4l2/v4l2_compat_manager.h | 4 +- > > > test/camera/camera_reconfigure.cpp | 1 + > > > test/camera/capture.cpp | 1 + > > > test/camera/configuration_default.cpp | 1 + > > > test/camera/configuration_set.cpp | 1 + > > > test/camera/statemachine.cpp | 1 + > > > test/gstreamer/gstreamer_test.h | 2 - > > > test/libtest/buffer_source.cpp | 2 + > > > test/libtest/buffer_source.h | 10 ++--- > > > test/libtest/camera_test.h | 6 +-- > > > test/mapped-buffer.cpp | 1 + > > > test/media_device/media_device_test.h | 6 +-- > > > test/serialization/serialization_test.h | 10 ++--- > > > test/v4l2_subdevice/v4l2_subdevice_test.h | 8 ++-- > > > test/v4l2_videodevice/buffer_sharing.cpp | 2 + > > > test/v4l2_videodevice/capture_async.cpp | 2 + > > > test/v4l2_videodevice/v4l2_videodevice_test.h | 14 +++---- > > > 27 files changed, 100 insertions(+), 107 deletions(-)
Hi Laurent, On Wed, Oct 20, 2021 at 11:39 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Wed, Oct 20, 2021 at 11:26:26AM +0900, Hirokazu Honda wrote: > > Hi Laurent, > > > > Would you merge this patch series? > > It has been merged already :-) > I couldn't find the commits somehow. Could you tell me the commit hash? -Hiro > > On Tue, Oct 5, 2021 at 6:51 PM Laurent Pinchart wrote: > > > On Tue, Oct 05, 2021 at 04:31:08PM +0900, Hirokazu Honda wrote: > > > > According to Google C++ style guide, using-directives (i.e. > > > > "using namespace foo") should not be used. However, libcamera has > > > > been using the scheme in many files. Removing using-directives > > > > entirely in the code definitely needs the dicussion. > > > > > > Indeed, that would need discussions :-) Part of the discussion would > > > likely touch the namespace name, libcamera is fairly long. > > > > > > > This patch series resolves the porblem of using using-directives > > > > in header file, which I expect I can handily get the agreement > > > > from developers. Using-directives in a header file is propagated > > > > to all the files including the header file. This pollutes > > > > namespace severely. > > > > > > Overall it looks fine to me, although I'm not sure it's such a big deal. > > > Yes, using directives are propagated, which is a bad idea in general, > > > but the header files below are all internal the the component they > > > relate to. The risk of namespace polution is thus more theoretical than > > > practical. > > > > > > This being said, we're already using this policy in the HAL headers, and > > > I don't object extending it to all headers. I'll side with the majority. > > > > > > > [1] https://google.github.io/styleguide/cppguide.html#Namespaces > > > > > > > > Hirokazu Honda (6): > > > > ipa: raspberrypi: agc: Remove using namespace in agc.hpp > > > > cam: Remove using namespace in stream_options.h > > > > qcam: Remove using namespace in header files > > > > v4l2: Remove using namespace in header files > > > > lc-compliance: Remove using namespace in header files > > > > test: Remove using namespace in header files > > > > > > > > src/cam/stream_options.h | 8 ++-- > > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 ++++ > > > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +-- > > > > src/lc-compliance/environment.cpp | 2 +- > > > > src/lc-compliance/environment.h | 8 ++-- > > > > src/qcam/dng_writer.h | 10 ++--- > > > > src/qcam/main.cpp | 2 +- > > > > src/qcam/main_window.h | 40 +++++++++---------- > > > > src/v4l2/v4l2_camera.h | 37 +++++++++-------- > > > > src/v4l2/v4l2_camera_proxy.h | 15 ++++--- > > > > src/v4l2/v4l2_compat_manager.h | 4 +- > > > > test/camera/camera_reconfigure.cpp | 1 + > > > > test/camera/capture.cpp | 1 + > > > > test/camera/configuration_default.cpp | 1 + > > > > test/camera/configuration_set.cpp | 1 + > > > > test/camera/statemachine.cpp | 1 + > > > > test/gstreamer/gstreamer_test.h | 2 - > > > > test/libtest/buffer_source.cpp | 2 + > > > > test/libtest/buffer_source.h | 10 ++--- > > > > test/libtest/camera_test.h | 6 +-- > > > > test/mapped-buffer.cpp | 1 + > > > > test/media_device/media_device_test.h | 6 +-- > > > > test/serialization/serialization_test.h | 10 ++--- > > > > test/v4l2_subdevice/v4l2_subdevice_test.h | 8 ++-- > > > > test/v4l2_videodevice/buffer_sharing.cpp | 2 + > > > > test/v4l2_videodevice/capture_async.cpp | 2 + > > > > test/v4l2_videodevice/v4l2_videodevice_test.h | 14 +++---- > > > > 27 files changed, 100 insertions(+), 107 deletions(-) > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Wed, Oct 20, 2021 at 12:01:52PM +0900, Hirokazu Honda wrote: > On Wed, Oct 20, 2021 at 11:39 AM Laurent Pinchart wrote: > > On Wed, Oct 20, 2021 at 11:26:26AM +0900, Hirokazu Honda wrote: > > > Hi Laurent, > > > > > > Would you merge this patch series? > > > > It has been merged already :-) > > I couldn't find the commits somehow. > Could you tell me the commit hash? 1526650fc781 test: Remove using namespace in header files af60569cbc16 lc-compliance: Remove using namespace in header files 4689b165720e v4l2: Remove using namespace in header files f277590d5cb1 qcam: Remove using namespace in header files 3b93746907df cam: Remove using namespace in stream_options.h ba7cf5d6544d ipa: raspberrypi: agc: Remove using namespace in agc.hpp > > > On Tue, Oct 5, 2021 at 6:51 PM Laurent Pinchart wrote: > > > > On Tue, Oct 05, 2021 at 04:31:08PM +0900, Hirokazu Honda wrote: > > > > > According to Google C++ style guide, using-directives (i.e. > > > > > "using namespace foo") should not be used. However, libcamera has > > > > > been using the scheme in many files. Removing using-directives > > > > > entirely in the code definitely needs the dicussion. > > > > > > > > Indeed, that would need discussions :-) Part of the discussion would > > > > likely touch the namespace name, libcamera is fairly long. > > > > > > > > > This patch series resolves the porblem of using using-directives > > > > > in header file, which I expect I can handily get the agreement > > > > > from developers. Using-directives in a header file is propagated > > > > > to all the files including the header file. This pollutes > > > > > namespace severely. > > > > > > > > Overall it looks fine to me, although I'm not sure it's such a big deal. > > > > Yes, using directives are propagated, which is a bad idea in general, > > > > but the header files below are all internal the the component they > > > > relate to. The risk of namespace polution is thus more theoretical than > > > > practical. > > > > > > > > This being said, we're already using this policy in the HAL headers, and > > > > I don't object extending it to all headers. I'll side with the majority. > > > > > > > > > [1] https://google.github.io/styleguide/cppguide.html#Namespaces > > > > > > > > > > Hirokazu Honda (6): > > > > > ipa: raspberrypi: agc: Remove using namespace in agc.hpp > > > > > cam: Remove using namespace in stream_options.h > > > > > qcam: Remove using namespace in header files > > > > > v4l2: Remove using namespace in header files > > > > > lc-compliance: Remove using namespace in header files > > > > > test: Remove using namespace in header files > > > > > > > > > > src/cam/stream_options.h | 8 ++-- > > > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 ++++ > > > > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +-- > > > > > src/lc-compliance/environment.cpp | 2 +- > > > > > src/lc-compliance/environment.h | 8 ++-- > > > > > src/qcam/dng_writer.h | 10 ++--- > > > > > src/qcam/main.cpp | 2 +- > > > > > src/qcam/main_window.h | 40 +++++++++---------- > > > > > src/v4l2/v4l2_camera.h | 37 +++++++++-------- > > > > > src/v4l2/v4l2_camera_proxy.h | 15 ++++--- > > > > > src/v4l2/v4l2_compat_manager.h | 4 +- > > > > > test/camera/camera_reconfigure.cpp | 1 + > > > > > test/camera/capture.cpp | 1 + > > > > > test/camera/configuration_default.cpp | 1 + > > > > > test/camera/configuration_set.cpp | 1 + > > > > > test/camera/statemachine.cpp | 1 + > > > > > test/gstreamer/gstreamer_test.h | 2 - > > > > > test/libtest/buffer_source.cpp | 2 + > > > > > test/libtest/buffer_source.h | 10 ++--- > > > > > test/libtest/camera_test.h | 6 +-- > > > > > test/mapped-buffer.cpp | 1 + > > > > > test/media_device/media_device_test.h | 6 +-- > > > > > test/serialization/serialization_test.h | 10 ++--- > > > > > test/v4l2_subdevice/v4l2_subdevice_test.h | 8 ++-- > > > > > test/v4l2_videodevice/buffer_sharing.cpp | 2 + > > > > > test/v4l2_videodevice/capture_async.cpp | 2 + > > > > > test/v4l2_videodevice/v4l2_videodevice_test.h | 14 +++---- > > > > > 27 files changed, 100 insertions(+), 107 deletions(-)
Hi Laurent, On Wed, Oct 20, 2021 at 12:09 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Wed, Oct 20, 2021 at 12:01:52PM +0900, Hirokazu Honda wrote: > > On Wed, Oct 20, 2021 at 11:39 AM Laurent Pinchart wrote: > > > On Wed, Oct 20, 2021 at 11:26:26AM +0900, Hirokazu Honda wrote: > > > > Hi Laurent, > > > > > > > > Would you merge this patch series? > > > > > > It has been merged already :-) > > > > I couldn't find the commits somehow. > > Could you tell me the commit hash? > > 1526650fc781 test: Remove using namespace in header files > af60569cbc16 lc-compliance: Remove using namespace in header files > 4689b165720e v4l2: Remove using namespace in header files > f277590d5cb1 qcam: Remove using namespace in header files > 3b93746907df cam: Remove using namespace in stream_options.h > ba7cf5d6544d ipa: raspberrypi: agc: Remove using namespace in agc.hpp > Thanks! I found them. > > > > On Tue, Oct 5, 2021 at 6:51 PM Laurent Pinchart wrote: > > > > > On Tue, Oct 05, 2021 at 04:31:08PM +0900, Hirokazu Honda wrote: > > > > > > According to Google C++ style guide, using-directives (i.e. > > > > > > "using namespace foo") should not be used. However, libcamera has > > > > > > been using the scheme in many files. Removing using-directives > > > > > > entirely in the code definitely needs the dicussion. > > > > > > > > > > Indeed, that would need discussions :-) Part of the discussion would > > > > > likely touch the namespace name, libcamera is fairly long. > > > > > > > > > > > This patch series resolves the porblem of using using-directives > > > > > > in header file, which I expect I can handily get the agreement > > > > > > from developers. Using-directives in a header file is propagated > > > > > > to all the files including the header file. This pollutes > > > > > > namespace severely. > > > > > > > > > > Overall it looks fine to me, although I'm not sure it's such a big deal. > > > > > Yes, using directives are propagated, which is a bad idea in general, > > > > > but the header files below are all internal the the component they > > > > > relate to. The risk of namespace polution is thus more theoretical than > > > > > practical. > > > > > > > > > > This being said, we're already using this policy in the HAL headers, and > > > > > I don't object extending it to all headers. I'll side with the majority. > > > > > > > > > > > [1] https://google.github.io/styleguide/cppguide.html#Namespaces > > > > > > > > > > > > Hirokazu Honda (6): > > > > > > ipa: raspberrypi: agc: Remove using namespace in agc.hpp > > > > > > cam: Remove using namespace in stream_options.h > > > > > > qcam: Remove using namespace in header files > > > > > > v4l2: Remove using namespace in header files > > > > > > lc-compliance: Remove using namespace in header files > > > > > > test: Remove using namespace in header files > > > > > > > > > > > > src/cam/stream_options.h | 8 ++-- > > > > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 ++++ > > > > > > src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +-- > > > > > > src/lc-compliance/environment.cpp | 2 +- > > > > > > src/lc-compliance/environment.h | 8 ++-- > > > > > > src/qcam/dng_writer.h | 10 ++--- > > > > > > src/qcam/main.cpp | 2 +- > > > > > > src/qcam/main_window.h | 40 +++++++++---------- > > > > > > src/v4l2/v4l2_camera.h | 37 +++++++++-------- > > > > > > src/v4l2/v4l2_camera_proxy.h | 15 ++++--- > > > > > > src/v4l2/v4l2_compat_manager.h | 4 +- > > > > > > test/camera/camera_reconfigure.cpp | 1 + > > > > > > test/camera/capture.cpp | 1 + > > > > > > test/camera/configuration_default.cpp | 1 + > > > > > > test/camera/configuration_set.cpp | 1 + > > > > > > test/camera/statemachine.cpp | 1 + > > > > > > test/gstreamer/gstreamer_test.h | 2 - > > > > > > test/libtest/buffer_source.cpp | 2 + > > > > > > test/libtest/buffer_source.h | 10 ++--- > > > > > > test/libtest/camera_test.h | 6 +-- > > > > > > test/mapped-buffer.cpp | 1 + > > > > > > test/media_device/media_device_test.h | 6 +-- > > > > > > test/serialization/serialization_test.h | 10 ++--- > > > > > > test/v4l2_subdevice/v4l2_subdevice_test.h | 8 ++-- > > > > > > test/v4l2_videodevice/buffer_sharing.cpp | 2 + > > > > > > test/v4l2_videodevice/capture_async.cpp | 2 + > > > > > > test/v4l2_videodevice/v4l2_videodevice_test.h | 14 +++---- > > > > > > 27 files changed, 100 insertions(+), 107 deletions(-) > > -- > Regards, > > Laurent Pinchart