| Message ID | 20250224185235.43381-5-mzamazal@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Mon, Feb 24, 2025 at 07:52:34PM +0100, Milan Zamazal wrote: > Let's make the autoformatter happy with thread.cpp formatting. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/base/thread.cpp | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > index 319bfda9..02128f23 100644 > --- a/src/libcamera/base/thread.cpp > +++ b/src/libcamera/base/thread.cpp > @@ -5,8 +5,6 @@ > * Thread support > */ > > -#include <libcamera/base/thread.h> > - This is done on purpose, see Documentation/coding-style.rst: For .cpp files, if the file implements an API declared in a header file, that header file shall be included first in order to ensure it is self-contained. > #include <atomic> > #include <list> > #include <optional> > @@ -20,6 +18,7 @@ > #include <libcamera/base/message.h> > #include <libcamera/base/mutex.h> > #include <libcamera/base/object.h> > +#include <libcamera/base/thread.h> > > /** > * \page thread Thread Support > @@ -657,7 +656,7 @@ void Thread::dispatchMessages(Message::Type type) > * the outer calls. > */ > if (!--data_->messages_.recursion_) { > - for (auto iter = messages.begin(); iter != messages.end(); ) { > + for (auto iter = messages.begin(); iter != messages.end();) { Hmmmm... I thought we standardized on the style with a space, but we have 6 occurrences with a space, and 15 without. Among those 15, 10 are in the software ISP though :-) I find the space more readable, but if there's no way to make clang-format happy about it, I'm OK dropping it. > if (!*iter) > iter = messages.erase(iter); > else
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Mon, Feb 24, 2025 at 07:52:34PM +0100, Milan Zamazal wrote: >> Let's make the autoformatter happy with thread.cpp formatting. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/base/thread.cpp | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp >> index 319bfda9..02128f23 100644 >> --- a/src/libcamera/base/thread.cpp >> +++ b/src/libcamera/base/thread.cpp >> @@ -5,8 +5,6 @@ >> * Thread support >> */ >> >> -#include <libcamera/base/thread.h> >> - > > This is done on purpose, see Documentation/coding-style.rst: > > For .cpp files, if the file implements an API declared in a header > file, that header file shall be included first in order to ensure it > is self-contained. Ah, right. It seems the autoformatter does the right thing for includes with quotes but not for those with angle brackets. I can see in .clang-format IncludeCategories: # Headers matching the name of the component are matched automatically. # Priority 1 so it doesn't look like we can do anything about it. >> #include <atomic> >> #include <list> >> #include <optional> >> @@ -20,6 +18,7 @@ >> #include <libcamera/base/message.h> >> #include <libcamera/base/mutex.h> >> #include <libcamera/base/object.h> >> +#include <libcamera/base/thread.h> >> >> /** >> * \page thread Thread Support >> @@ -657,7 +656,7 @@ void Thread::dispatchMessages(Message::Type type) >> * the outer calls. >> */ >> if (!--data_->messages_.recursion_) { >> - for (auto iter = messages.begin(); iter != messages.end(); ) { >> + for (auto iter = messages.begin(); iter != messages.end();) { > > Hmmmm... I thought we standardized on the style with a space, but we > have 6 occurrences with a space, and 15 without. Among those 15, 10 are > in the software ISP though :-) Maybe because of the autoformatter assistance? > I find the space more readable, but if there's no way to make > clang-format happy about it, I'm OK dropping it. I don't mind either way as long as I don't have to override changes made by the formatter all the time. :-) >> if (!*iter) >> iter = messages.erase(iter); >> else
On Mon, Feb 24, 2025 at 09:34:28PM +0100, Milan Zamazal wrote: > Laurent Pinchart writes: > > On Mon, Feb 24, 2025 at 07:52:34PM +0100, Milan Zamazal wrote: > >> Let's make the autoformatter happy with thread.cpp formatting. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/libcamera/base/thread.cpp | 5 ++--- > >> 1 file changed, 2 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > >> index 319bfda9..02128f23 100644 > >> --- a/src/libcamera/base/thread.cpp > >> +++ b/src/libcamera/base/thread.cpp > >> @@ -5,8 +5,6 @@ > >> * Thread support > >> */ > >> > >> -#include <libcamera/base/thread.h> > >> - > > > > This is done on purpose, see Documentation/coding-style.rst: > > > > For .cpp files, if the file implements an API declared in a header > > file, that header file shall be included first in order to ensure it > > is self-contained. > > Ah, right. It seems the autoformatter does the right thing for includes > with quotes but not for those with angle brackets. I can see in > .clang-format > > IncludeCategories: > # Headers matching the name of the component are matched automatically. > # Priority 1 > > so it doesn't look like we can do anything about it. > > >> #include <atomic> > >> #include <list> > >> #include <optional> > >> @@ -20,6 +18,7 @@ > >> #include <libcamera/base/message.h> > >> #include <libcamera/base/mutex.h> > >> #include <libcamera/base/object.h> > >> +#include <libcamera/base/thread.h> > >> > >> /** > >> * \page thread Thread Support > >> @@ -657,7 +656,7 @@ void Thread::dispatchMessages(Message::Type type) > >> * the outer calls. > >> */ > >> if (!--data_->messages_.recursion_) { > >> - for (auto iter = messages.begin(); iter != messages.end(); ) { > >> + for (auto iter = messages.begin(); iter != messages.end();) { > > > > Hmmmm... I thought we standardized on the style with a space, but we > > have 6 occurrences with a space, and 15 without. Among those 15, 10 are > > in the software ISP though :-) > > Maybe because of the autoformatter assistance? > > > I find the space more readable, but if there's no way to make > > clang-format happy about it, I'm OK dropping it. > > I don't mind either way as long as I don't have to override changes made > by the formatter all the time. :-) I didn't find a suitable clang-format option after a cursory look. If you can't see one either, I'm fine changing this. As the header change from this patch needs to be dropped, you could submit a v2 that drops the space after the semicolon in all locations (I just ran "git grep '; )'" to locate them). That can be sent as patch of its own, separate from this series. > >> if (!*iter) > >> iter = messages.erase(iter); > >> else
diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp index 319bfda9..02128f23 100644 --- a/src/libcamera/base/thread.cpp +++ b/src/libcamera/base/thread.cpp @@ -5,8 +5,6 @@ * Thread support */ -#include <libcamera/base/thread.h> - #include <atomic> #include <list> #include <optional> @@ -20,6 +18,7 @@ #include <libcamera/base/message.h> #include <libcamera/base/mutex.h> #include <libcamera/base/object.h> +#include <libcamera/base/thread.h> /** * \page thread Thread Support @@ -657,7 +656,7 @@ void Thread::dispatchMessages(Message::Type type) * the outer calls. */ if (!--data_->messages_.recursion_) { - for (auto iter = messages.begin(); iter != messages.end(); ) { + for (auto iter = messages.begin(); iter != messages.end();) { if (!*iter) iter = messages.erase(iter); else
Let's make the autoformatter happy with thread.cpp formatting. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/base/thread.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)