[libcamera-devel] [RFC PATCH] libcamera: Rename 'method' to 'function'

[Laurent Pinchart]

Hi Kieran,

On Fri, Aug 06, 2021 at 11:26:26AM +0100, Kieran Bingham wrote:
> On 06/08/2021 11:10, Laurent Pinchart wrote:
> > On Fri, Aug 06, 2021 at 10:57:50AM +0100, Kieran Bingham wrote:
> >> On 05/08/2021 17:35, Laurent Pinchart wrote:
> >>> Usage of 'method' to refer to member functions comes from Java. The C++
> >>> standard uses the term 'function' only. Replace 'method' with 'function'
> >>> or 'member function' through the whole code base and documentation.
> >>
> >> Ouch ;-)
> >>
> >> I think method is widely known as a 'function of a class' ... but indeed
> >> that's also more correctly known as a member function I think.
> >>
> >> So - using 'function' over 'method' is perhaps more correct for C++.
> >>
> >>
> >> I won't nitpick over this for specific reviews, as long as it doesn't
> >> break compilation, and checkstyle is happy, I'm sure you've dealt with
> >> line lengths and reflows accordingly.
> > 
> > Paul went through the patch and found a typo in a comment :-)
> 
> Perfect, see - I don't need to hehe
> 
> >> Acked-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>
> >> I do wonder if you might consider adding the rule to checkstyle though ...
> > 
> > Yes, but I think we should rename BoundMethod first in that case.
> > Otherwise there will be too many false positives.
> > 
> > Work on BoundMethod is on my todo list, I'd like to investigate if the
> > custom implementation could be replaced (or at least simplified) with
> > usage of C++17 features such as std::apply, but that's not high
> > priority.
> 
> Is BoundMethod exposed as public libcamera API - which would currently
> prevent C++17 ..?

Correct, but not forever. Our main blocker is
https://bugs.chromium.org/p/chromium/issues/detail?id=752720, and 9 out
of its 10 blockers have been solved.

> >> But - perhaps that might just be a generic 's/method/function/' on the
> >> whole patch content to see if it picks up any changes?
> >>
> >> Essentially it's a custom spell check correction ... hrm ...
> >>
> >>> The BoundMethod and Object::invokeMethod() are left as-is here, and will
> >>> be addressed separately.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>> I've carried this in my tree for too long, and it's very annoying to
> >>> rebase as you can imagine, so I thought I'd sent it out to get it
> >>> accepted or rejected, but out of its current limbo.
> >>> ---
> >>>  Documentation/coding-style.rst                |   2 +-
> >>>  Documentation/docs.rst                        |   2 +-
> >>>  .../guides/application-developer.rst          |   4 +-
> >>>  Documentation/guides/ipa.rst                  |  10 +-
> >>>  Documentation/guides/pipeline-handler.rst     | 102 +++++++++---------
> >>>  include/libcamera/ipa/core.mojom              |   2 +-
> >>>  include/libcamera/ipa/raspberrypi.mojom       |  31 +++---
> >>>  src/android/camera_device.cpp                 |   2 +-
> >>>  src/android/jpeg/exif.cpp                     |   2 +-
> >>>  src/ipa/ipu3/ipu3.cpp                         |   2 +-
> >>>  src/ipa/libipa/camera_sensor_helper.cpp       |   2 +-
> >>>  src/ipa/raspberrypi/cam_helper.cpp            |   4 +-
> >>>  src/ipa/raspberrypi/cam_helper.hpp            |  14 +--
> >>>  src/ipa/raspberrypi/controller/controller.hpp |   4 +-
> >>>  src/ipa/raspberrypi/controller/rpi/alsc.hpp   |   2 +-
> >>>  .../raspberrypi/controller/rpi/sharpen.cpp    |   2 +-
> >>>  src/ipa/raspberrypi/md_parser.hpp             |   4 +-
> >>>  src/ipa/raspberrypi/raspberrypi.cpp           |   2 +-
> >>>  src/libcamera/base/event_dispatcher_poll.cpp  |   2 +-
> >>>  src/libcamera/base/log.cpp                    |  10 +-
> >>>  src/libcamera/base/message.cpp                |   6 +-
> >>>  src/libcamera/base/object.cpp                 |  22 ++--
> >>>  src/libcamera/base/semaphore.cpp              |  16 +--
> >>>  src/libcamera/base/signal.cpp                 |   2 +-
> >>>  src/libcamera/base/thread.cpp                 |  40 +++----
> >>>  src/libcamera/byte_stream_buffer.cpp          |   9 +-
> >>>  src/libcamera/camera.cpp                      |  24 ++---
> >>>  src/libcamera/camera_manager.cpp              |   2 +-
> >>>  src/libcamera/camera_sensor.cpp               |  24 ++---
> >>>  src/libcamera/control_serializer.cpp          |   6 +-
> >>>  src/libcamera/control_validator.cpp           |   2 +-
> >>>  src/libcamera/controls.cpp                    |  20 ++--
> >>>  src/libcamera/device_enumerator.cpp           |   2 +-
> >>>  src/libcamera/file_descriptor.cpp             |  14 +--
> >>>  src/libcamera/formats.cpp                     |   2 +-
> >>>  src/libcamera/framebuffer.cpp                 |  10 +-
> >>>  src/libcamera/framebuffer_allocator.cpp       |   4 +-
> >>>  src/libcamera/geometry.cpp                    |   2 +-
> >>>  src/libcamera/ipa_interface.cpp               |  14 +--
> >>>  src/libcamera/ipa_manager.cpp                 |  11 +-
> >>>  src/libcamera/ipa_module.cpp                  |  12 +--
> >>>  src/libcamera/ipa_proxy.cpp                   |   4 +-
> >>>  src/libcamera/ipc_unixsocket.cpp              |  22 ++--
> >>>  src/libcamera/media_device.cpp                |   4 +-
> >>>  src/libcamera/pipeline/ipu3/imgu.cpp          |   4 +-
> >>>  src/libcamera/pipeline_handler.cpp            |  58 +++++-----
> >>>  src/libcamera/process.cpp                     |  10 +-
> >>>  src/libcamera/request.cpp                     |  11 +-
> >>>  src/libcamera/stream.cpp                      |  17 +--
> >>>  src/libcamera/v4l2_device.cpp                 |  20 ++--
> >>>  src/libcamera/v4l2_videodevice.cpp            |  16 +--
> >>>  test/media_device/media_device_link_test.cpp  |   2 +-
> >>>  test/signal.cpp                               |   4 +-
> >>>  53 files changed, 313 insertions(+), 308 deletions(-)
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart