[libcamera-devel,v8,2/4] cam: event_loop: Rename addEvent to addFDEvent
diff mbox series

Message ID 20220505151851.2421-2-ecurtin@redhat.com
State Superseded
Headers show
Series
  • [libcamera-devel,v8,1/4] cam: event_loop: Add timer events to event loop
Related show

Commit Message

Eric Curtin May 5, 2022, 3:18 p.m. UTC 
Since we now have two types of events addFDEvent and addTimerEvent.

Signed-off-by: Eric Curtin <ecurtin@redhat.com>
---
 src/cam/drm.cpp        | 4 ++--
 src/cam/event_loop.cpp | 4 ++--
 src/cam/event_loop.h   | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

Comments

Kieran Bingham May 17, 2022, 11:22 p.m. UTC | #1 
Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:49)
> Since we now have two types of events addFDEvent and addTimerEvent.
> 

I think this could be expanded a little:

"""
With the addition of addTimerEvent, the naming of addEvent is specific
to the management of an fd, while the naming is generic.

Update the name to make the naming scheme consistent in specifying the
type of event to be added.
"""

But that's optional, and could be adapted as best fits.

It looks like the change is fine so:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> ---
>  src/cam/drm.cpp        | 4 ++--
>  src/cam/event_loop.cpp | 4 ++--
>  src/cam/event_loop.h   | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> index 46e34eb5..421cd61a 100644
> --- a/src/cam/drm.cpp
> +++ b/src/cam/drm.cpp
> @@ -432,8 +432,8 @@ int Device::init()
>         if (ret < 0)
>                 return ret;
>  
> -       EventLoop::instance()->addEvent(fd_, EventLoop::Read,
> -                                       std::bind(&Device::drmEvent, this));
> +       EventLoop::instance()->addFDEvent(fd_, EventLoop::Read,
> +                                         std::bind(&Device::drmEvent, this));
>  
>         return 0;
>  }
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index 181c971c..0c2176c7 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -64,8 +64,8 @@ void EventLoop::callLater(const std::function<void()> &func)
>         event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
>  }
>  
> -void EventLoop::addEvent(int fd, EventType type,
> -                        const std::function<void()> &callback)
> +void EventLoop::addFDEvent(int fd, EventType type,
> +                          const std::function<void()> &callback)
>  {
>         std::unique_ptr<Event> event = std::make_unique<Event>(callback);
>         short events = (type & Read ? EV_READ : 0)
> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> index 89215dce..d921cd8f 100644
> --- a/src/cam/event_loop.h
> +++ b/src/cam/event_loop.h
> @@ -36,8 +36,8 @@ public:
>  
>         void callLater(const std::function<void()> &func);
>  
> -       void addEvent(int fd, EventType type,
> -                     const std::function<void()> &handler);
> +       void addFDEvent(int fd, EventType type,
> +                       const std::function<void()> &handler);
>  
>         void addTimerEvent(const std::chrono::milliseconds d,
>                            const std::function<void()> &handler);
> -- 
> 2.35.1
>
Laurent Pinchart May 18, 2022, 7:28 p.m. UTC | #2 
On Wed, May 18, 2022 at 12:22:03AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:49)
> > Since we now have two types of events addFDEvent and addTimerEvent.
> 
> I think this could be expanded a little:
> 
> """
> With the addition of addTimerEvent, the naming of addEvent is specific
> to the management of an fd, while the naming is generic.
> 
> Update the name to make the naming scheme consistent in specifying the
> type of event to be added.
> """

Looks good to me.

I would actually have moved this patch first in the series, but that
requires rewording the commit message, it's not worth the effort at this
point.

> But that's optional, and could be adapted as best fits.
> 
> It looks like the change is fine so:
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > ---
> >  src/cam/drm.cpp        | 4 ++--
> >  src/cam/event_loop.cpp | 4 ++--
> >  src/cam/event_loop.h   | 4 ++--
> >  3 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > index 46e34eb5..421cd61a 100644
> > --- a/src/cam/drm.cpp
> > +++ b/src/cam/drm.cpp
> > @@ -432,8 +432,8 @@ int Device::init()
> >         if (ret < 0)
> >                 return ret;
> >  
> > -       EventLoop::instance()->addEvent(fd_, EventLoop::Read,
> > -                                       std::bind(&Device::drmEvent, this));
> > +       EventLoop::instance()->addFDEvent(fd_, EventLoop::Read,
> > +                                         std::bind(&Device::drmEvent, this));
> >  
> >         return 0;
> >  }
> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > index 181c971c..0c2176c7 100644
> > --- a/src/cam/event_loop.cpp
> > +++ b/src/cam/event_loop.cpp
> > @@ -64,8 +64,8 @@ void EventLoop::callLater(const std::function<void()> &func)
> >         event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
> >  }
> >  
> > -void EventLoop::addEvent(int fd, EventType type,
> > -                        const std::function<void()> &callback)
> > +void EventLoop::addFDEvent(int fd, EventType type,

The function should be named addFdEvent().

With this changed and the commit message updated,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +                          const std::function<void()> &callback)
> >  {
> >         std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> >         short events = (type & Read ? EV_READ : 0)
> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> > index 89215dce..d921cd8f 100644
> > --- a/src/cam/event_loop.h
> > +++ b/src/cam/event_loop.h
> > @@ -36,8 +36,8 @@ public:
> >  
> >         void callLater(const std::function<void()> &func);
> >  
> > -       void addEvent(int fd, EventType type,
> > -                     const std::function<void()> &handler);
> > +       void addFDEvent(int fd, EventType type,
> > +                       const std::function<void()> &handler);
> >  
> >         void addTimerEvent(const std::chrono::milliseconds d,
> >                            const std::function<void()> &handler);

Patch
diff mbox series 

diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
index 46e34eb5..421cd61a 100644
--- a/src/cam/drm.cpp
+++ b/src/cam/drm.cpp
@@ -432,8 +432,8 @@  int Device::init()
 	if (ret < 0)
 		return ret;
 
-	EventLoop::instance()->addEvent(fd_, EventLoop::Read,
-					std::bind(&Device::drmEvent, this));
+	EventLoop::instance()->addFDEvent(fd_, EventLoop::Read,
+					  std::bind(&Device::drmEvent, this));
 
 	return 0;
 }
diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
index 181c971c..0c2176c7 100644
--- a/src/cam/event_loop.cpp
+++ b/src/cam/event_loop.cpp
@@ -64,8 +64,8 @@  void EventLoop::callLater(const std::function<void()> &func)
 	event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
 }
 
-void EventLoop::addEvent(int fd, EventType type,
-			 const std::function<void()> &callback)
+void EventLoop::addFDEvent(int fd, EventType type,
+			   const std::function<void()> &callback)
 {
 	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
 	short events = (type & Read ? EV_READ : 0)
diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
index 89215dce..d921cd8f 100644
--- a/src/cam/event_loop.h
+++ b/src/cam/event_loop.h
@@ -36,8 +36,8 @@  public:
 
 	void callLater(const std::function<void()> &func);
 
-	void addEvent(int fd, EventType type,
-		      const std::function<void()> &handler);
+	void addFDEvent(int fd, EventType type,
+			const std::function<void()> &handler);
 
 	void addTimerEvent(const std::chrono::milliseconds d,
 			   const std::function<void()> &handler);