[libcamera-devel] [PATCH] libcamera: event_dispatcher_poll: Simplify range iterator

[Laurent Pinchart]

Hi Kieran,

On Tue, Jun 25, 2019 at 06:13:10PM +0100, Kieran Bingham wrote:
> On 25/06/2019 14:32, Laurent Pinchart wrote:
> > Hi Kieran,
> > 
> > Thank you for the patch.
> > 
> > I would write the subject as "Fix compilation warning" or something
> > along those lines.
> 
> I think the subject should state *what* it does, rather than /why/.

Yes, but there's always a bit of the 'why' in the 'what'. Otherwise you
could write "remove characters from a line" :-)

Don't forget that it's common to skim through commit just looking at the
subject line, so mentioning the main purpose of the commit there is
useful.

> To fix a compilation warning (or error as we use -Werror) is the reason
> why we fix it :)
> 
> How about:
> 
> libcamera: event_dispatcher_poll: Remove struct keyword from for-range

That works too.

> I'd like to make that 'remove redundant struct keyword' but I've run out
> of title space :D
> 
> > On Tue, Jun 25, 2019 at 02:15:08PM +0100, Kieran Bingham wrote:
> >> GCCv6 and GCCv7 take objections to declaring a struct type when
> >> using a range based iterator. This issue does not appear in GCCv8
> >>
> >>     event_dispatcher_poll.cpp:231:13: error: types may not be defined
> >> 	in a for-range-declaration [-Werror]
> >>
> >> 		for (const struct pollfd &pfd : pollfds) {
> >> 		           ^~~~~~
> >>
> >> 	cc1plus: all warnings being treated as errors
> >>
> >> Removing the keyword 'struct' ensures that the compiler does not try to
> >> declare the type, and instead uses the type as already defined by the
> >> relevant poll.h header.
> > 
> > I can't reproduce this with gcc 6.4.0 or 7.4.0, I wonder if it could be
> > related to the C library. You may want to mention the exact compiler
> > versions that resulted in the error.
> 
> Using the following code sample, I have tested a range of compilers
> using godbolt.org:
> 
> #include <vector>
> #include <poll.h>
> 
> void processNotifiers(const std::vector<struct pollfd> &pollfds) {
>     	for (const struct pollfd &pfd : pollfds) {
> 
>         }
> }
> 
> So actually I have misinterpreted the issue. I thought it was a newly
> introduced compiler warning. In fact, old compilers fail - while newer
> compilers accept our current syntax.
> 
> 
> Godbolt shows that this failure occurs at the following versions:
> 
>  v4.9.3 fails
>  v4.9.4 fails
> 
>  v5.1 fails
>  v5.2 fails
>  v5.3 fails
>  v5.4 fails
>  v5.5 fails
> 
>  v6.2 fails
>  v6.3 fails
> 
>  v6.4 supported
>  (and so are the versions following)
> 
> The code sample can be explored at this godbolt short-link:
>    https://godbolt.org/z/mV6ita

I've tested gcc 5.4.0 here and it doesn't produce that warning :-S I get
a

struct.cpp: In function ‘void processNotifiers(const std::vector<pollfd>&)’:
struct.cpp:6:28: warning: unused variable ‘pfd’ [-Wunused-variable]
  for (const struct pollfd &pfd : pollfds) {
                            ^

which is expected, and after modifying your sample code to

#include <vector>
#include <poll.h>

void func(const struct pollfd &pfd);

void processNotifiers(const std::vector<struct pollfd> &pollfds)
{
        for (const struct pollfd &pfd : pollfds) {
                func(pfd);
        }
}

the code compile cleanly with

$ g++-5.4.0 -W -Wall -std=c++11 -c -o struct struct.cpp

Have you noticed that the error reported by the above godbolt.org link
for v5.4 is

<source>: In function 'void processNotifiers(const std::vector<pollfd>&)':
<source>:5:17: error: types may not be defined in a for-range-declaration [-Werror]
      for (const struct pollfd &pfd : pollfds) {
                 ^~~~~~
cc1plus: all warnings being treated as errors
Compiler returned: 1

and goes away when you add -std=c++11 ? Only 6.2 and 6.3 seem to suffer
from the struct in range loop issue.

> > In any case,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Thanks, I've updated the commit message a fair bit so I'll send a v2 anyway.
> --
> Kieran
> 
> 
> > 
> >> Reported-by: [autobuild.buildroot.net] Thomas Petazzoni <thomas.petazzoni at bootlin.com>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  src/libcamera/event_dispatcher_poll.cpp | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp
> >> index 0ff99fce47ab..df9dffb2326c 100644
> >> --- a/src/libcamera/event_dispatcher_poll.cpp
> >> +++ b/src/libcamera/event_dispatcher_poll.cpp
> >> @@ -241,7 +241,7 @@ void EventDispatcherPoll::processNotifiers(const std::vector<struct pollfd> &pol
> >>  		{ EventNotifier::Exception, POLLPRI },
> >>  	};
> >>  
> >> -	for (const struct pollfd &pfd : pollfds) {
> >> +	for (const pollfd &pfd : pollfds) {
> >>  		auto iter = notifiers_.find(pfd.fd);
> >>  		ASSERT(iter != notifiers_.end());
> >>  

-- 
Regards,

Laurent Pinchart