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

[Kieran Bingham]

Hi Laurent,

On 25/06/2019 21:27, Kieran Bingham wrote:
> 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
>>

Aha - I did think it strange that you posted the same error message
here, but I seem to have glossed over that fact as a potential copy
paste error - hence my mis-representing that the same fault occurs
before 6.2...


Clearly, the message above was supposed to show:

> 
> <source>:5:38: error: range-based 'for' loops only available with -std=c++11 or -std=gnu++11 [-Werror]
> 
>       for (const struct pollfd &pfd : pollfds) {

Which clarifies the commit message for the v2.

I'll drop that sentence from the commit and push.

--
Regards

Kieran


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