| Message ID | 20240805143654.20870-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent On 05/08/2024 15:36, Laurent Pinchart wrote: > Two classes that have both public and internal headers, namely Camera > and Request, make only their public header visible to Doxygen through a > file directive. Fix them. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> How did you notice this, out of curiosity? I never saw any Doxygen messages about it. > --- > src/libcamera/camera.cpp | 5 +++++ > src/libcamera/request.cpp | 5 +++++ > 2 files changed, 10 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 67f3490133b2..f89510ea0472 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -117,6 +117,11 @@ > * of view is affected by the pipeline. > */ > > +/** > + * \file libcamera/internal/camera.h > + * \brief Internal camera device handling > + */ > + > namespace libcamera { > > LOG_DECLARE_CATEGORY(Camera) > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index cfb451e908da..fdf12c1e9088 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -28,6 +28,11 @@ > * \brief Describes a frame capture request to be processed by a camera > */ > > +/** > + * \file libcamera/internal/request.h > + * \brief Internal support for request handling > + */ > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(Request)
On Tue, Aug 06, 2024 at 02:34:56PM +0100, Daniel Scally wrote: > On 05/08/2024 15:36, Laurent Pinchart wrote: > > Two classes that have both public and internal headers, namely Camera > > and Request, make only their public header visible to Doxygen through a > > file directive. Fix them. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > > How did you notice this, out of curiosity? I never saw any Doxygen messages about it. Just by chance, when looking at the four classes that have public and internal headers of the same name, I realized there was a discrepancy. > > --- > > src/libcamera/camera.cpp | 5 +++++ > > src/libcamera/request.cpp | 5 +++++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 67f3490133b2..f89510ea0472 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -117,6 +117,11 @@ > > * of view is affected by the pipeline. > > */ > > > > +/** > > + * \file libcamera/internal/camera.h > > + * \brief Internal camera device handling > > + */ > > + > > namespace libcamera { > > > > LOG_DECLARE_CATEGORY(Camera) > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index cfb451e908da..fdf12c1e9088 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -28,6 +28,11 @@ > > * \brief Describes a frame capture request to be processed by a camera > > */ > > > > +/** > > + * \file libcamera/internal/request.h > > + * \brief Internal support for request handling > > + */ > > + > > namespace libcamera { > > > > LOG_DEFINE_CATEGORY(Request)
Quoting Laurent Pinchart (2024-08-06 14:39:38) > On Tue, Aug 06, 2024 at 02:34:56PM +0100, Daniel Scally wrote: > > On 05/08/2024 15:36, Laurent Pinchart wrote: > > > Two classes that have both public and internal headers, namely Camera > > > and Request, make only their public header visible to Doxygen through a > > > file directive. Fix them. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > > > > How did you notice this, out of curiosity? I never saw any Doxygen messages about it. > > Just by chance, when looking at the four classes that have public and > internal headers of the same name, I realized there was a discrepancy. > > > > --- > > > src/libcamera/camera.cpp | 5 +++++ > > > src/libcamera/request.cpp | 5 +++++ > > > 2 files changed, 10 insertions(+) > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index 67f3490133b2..f89510ea0472 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -117,6 +117,11 @@ > > > * of view is affected by the pipeline. > > > */ > > > > > > +/** > > > + * \file libcamera/internal/camera.h > > > + * \brief Internal camera device handling > > > + */ So the file directives cause doxygen to incoud Based on https://www.doxygen.nl/manual/commands.html#cmdfile the file directive only applies to the comment block it's in as I understand it. So this is 'only' adding a brief to the internal headers? Or is it doing more like actually pulling the file into the documentation? Either way, it seems consistent, and meets our coding style requirement "Every documented header file shall have a \file documentation block in the .cpp source file." Out of curiousity - what happens if the path is not unique? Is there a search order? Is a file in the same location as the .cpp chosen first? A quick check highlights : kbingham@Monstersaurus:~/iob/libcamera/libcamera-clean$ gg "\\\\file " | sed 's/.*file //' | sort | uniq -c | grep "2 " 2 agc.h 2 awb.h 2 blc.h 2 delayed_controls.h 2 ipa_context.h Does any action need to be taken on those files? Hang on - I'm on 2/18 - I should probably check the rest of the series before sending myself on rabbit holes ... ;-) Anyway, for this patch Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + > > > namespace libcamera { > > > > > > LOG_DECLARE_CATEGORY(Camera) > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > index cfb451e908da..fdf12c1e9088 100644 > > > --- a/src/libcamera/request.cpp > > > +++ b/src/libcamera/request.cpp > > > @@ -28,6 +28,11 @@ > > > * \brief Describes a frame capture request to be processed by a camera > > > */ > > > > > > +/** > > > + * \file libcamera/internal/request.h > > > + * \brief Internal support for request handling > > > + */ > > > + > > > namespace libcamera { > > > > > > LOG_DEFINE_CATEGORY(Request) > > -- > Regards, > > Laurent Pinchart
On Wed, Aug 07, 2024 at 05:59:16AM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2024-08-06 14:39:38) > > On Tue, Aug 06, 2024 at 02:34:56PM +0100, Daniel Scally wrote: > > > On 05/08/2024 15:36, Laurent Pinchart wrote: > > > > Two classes that have both public and internal headers, namely Camera > > > > and Request, make only their public header visible to Doxygen through a > > > > file directive. Fix them. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > > > > > > How did you notice this, out of curiosity? I never saw any Doxygen messages about it. > > > > Just by chance, when looking at the four classes that have public and > > internal headers of the same name, I realized there was a discrepancy. > > > > > > --- > > > > src/libcamera/camera.cpp | 5 +++++ > > > > src/libcamera/request.cpp | 5 +++++ > > > > 2 files changed, 10 insertions(+) > > > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > index 67f3490133b2..f89510ea0472 100644 > > > > --- a/src/libcamera/camera.cpp > > > > +++ b/src/libcamera/camera.cpp > > > > @@ -117,6 +117,11 @@ > > > > * of view is affected by the pipeline. > > > > */ > > > > > > > > +/** > > > > + * \file libcamera/internal/camera.h > > > > + * \brief Internal camera device handling > > > > + */ > > So the file directives cause doxygen to incoud > > Based on https://www.doxygen.nl/manual/commands.html#cmdfile the file > directive only applies to the comment block it's in as I understand it. > So this is 'only' adding a brief to the internal headers? Or is it doing > more like actually pulling the file into the documentation? I don't remember exactly why the file directive is needed, I just recall it is for some reason that matters :-) > Either way, it seems consistent, and meets our coding style requirement > "Every documented header file shall have a \file documentation block in > the .cpp source file." > > Out of curiousity - what happens if the path is not unique? Is there a > search order? Is a file in the same location as the .cpp chosen first? Doxygen complains in that case. > A quick check highlights : > > kbingham@Monstersaurus:~/iob/libcamera/libcamera-clean$ gg "\\\\file " | sed 's/.*file //' | sort | uniq -c | grep "2 " > 2 agc.h > 2 awb.h > 2 blc.h > 2 delayed_controls.h > 2 ipa_context.h > > Does any action need to be taken on those files? At least one of each isn't part of the documentation build, so we should be fine for now. > Hang on - I'm on 2/18 - I should probably check the rest of the series > before sending myself on rabbit holes ... ;-) > > Anyway, for this patch > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > + > > > > namespace libcamera { > > > > > > > > LOG_DECLARE_CATEGORY(Camera) > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > > index cfb451e908da..fdf12c1e9088 100644 > > > > --- a/src/libcamera/request.cpp > > > > +++ b/src/libcamera/request.cpp > > > > @@ -28,6 +28,11 @@ > > > > * \brief Describes a frame capture request to be processed by a camera > > > > */ > > > > > > > > +/** > > > > + * \file libcamera/internal/request.h > > > > + * \brief Internal support for request handling > > > > + */ > > > > + > > > > namespace libcamera { > > > > > > > > LOG_DEFINE_CATEGORY(Request)
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 67f3490133b2..f89510ea0472 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -117,6 +117,11 @@ * of view is affected by the pipeline. */ +/** + * \file libcamera/internal/camera.h + * \brief Internal camera device handling + */ + namespace libcamera { LOG_DECLARE_CATEGORY(Camera) diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index cfb451e908da..fdf12c1e9088 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -28,6 +28,11 @@ * \brief Describes a frame capture request to be processed by a camera */ +/** + * \file libcamera/internal/request.h + * \brief Internal support for request handling + */ + namespace libcamera { LOG_DEFINE_CATEGORY(Request)
Two classes that have both public and internal headers, namely Camera and Request, make only their public header visible to Doxygen through a file directive. Fix them. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/camera.cpp | 5 +++++ src/libcamera/request.cpp | 5 +++++ 2 files changed, 10 insertions(+)