| Message ID | 20190625131508.31263-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | caf25dc5cfd11b965316f02610d49ae3d886716b |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. I would write the subject as "Fix compilation warning" or something along those lines. 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. In any case, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reported-by: [autobuild.buildroot.net] Thomas Petazzoni <thomas.petazzoni@bootlin.com> > Signed-off-by: Kieran Bingham <kieran.bingham@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()); >
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@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@bootlin.com> >> Signed-off-by: Kieran Bingham <kieran.bingham@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()); >> >
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@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@bootlin.com> > >> Signed-off-by: Kieran Bingham <kieran.bingham@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()); > >>
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@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@bootlin.com> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@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()); >>>> >
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@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@bootlin.com> >>>>> Signed-off-by: Kieran Bingham <kieran.bingham@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()); >>>>> >> >
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());
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. Reported-by: [autobuild.buildroot.net] Thomas Petazzoni <thomas.petazzoni@bootlin.com> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/event_dispatcher_poll.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)