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

[Kieran Bingham]

Hi Laurent,

On 25/06/2019 18:29, Laurent Pinchart wrote:
> 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.

Yes, indeed - the -std=c++11 does seem to have an effect here, leaving
only 6.2 and 6.3 with issues.

So we're getting into a very specific compiler corner case :-S

Anyway - best not to dwell on this any longer, this issue has cost way
too much of my time. Lets add a fix and move on, and I'll send an update
for the buildroot integration.

--
Regards

Kieran



>>> In any case,
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>6563363d1a0665e3f37281d4ce4186f482f70212
>> 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