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

[Kieran Bingham]

Hi Laurent,

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/.

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

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




> 
> 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
--
Kieran