[RFC,v2,3/9] libcamera: base: log: Remove `LogMessage::init()`
diff mbox series

Message ID 20250130195811.1230581-4-pobrn@protonmail.com
State New
Headers show
Series
  • libcamera: base: log: Misc. changes
Related show

Commit Message

Barnabás Pőcze Jan. 30, 2025, 7:58 p.m. UTC
It is a short function that can be merged into the constructor with
essentially no change in observable behaviour, so do that.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/base/log.h |  2 --
 src/libcamera/base/log.cpp   | 13 +++----------
 2 files changed, 3 insertions(+), 12 deletions(-)

Comments

Jacopo Mondi Feb. 3, 2025, 4:30 p.m. UTC | #1
Hi Barnabás

On Thu, Jan 30, 2025 at 07:58:29PM +0000, Barnabás Pőcze wrote:
> It is a short function that can be merged into the constructor with
> essentially no change in observable behaviour, so do that.
>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/base/log.h |  2 --
>  src/libcamera/base/log.cpp   | 13 +++----------
>  2 files changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> index b3050eedb..6d2c93019 100644
> --- a/include/libcamera/base/log.h
> +++ b/include/libcamera/base/log.h
> @@ -75,8 +75,6 @@ public:
>  private:
>  	LIBCAMERA_DISABLE_COPY_AND_MOVE(LogMessage)
>
> -	void init(const char *fileName, unsigned int line);
> -
>  	std::ostringstream msgStream_;
>  	const LogCategory &category_;
>  	LogSeverity severity_;
> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> index 31053aee1..51a5cd470 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -875,19 +875,12 @@ const LogCategory &LogCategory::defaultCategory()
>  LogMessage::LogMessage(const char *fileName, unsigned int line,
>  		       const LogCategory &category, LogSeverity severity,
>  		       const std::string &prefix)
> -	: category_(category), severity_(severity), prefix_(prefix)
> +	: category_(category), severity_(severity),
> +	  timestamp_(utils::clock::now()), prefix_(prefix)
>  {
> -	init(fileName, line);
> -}
> -
> -void LogMessage::init(const char *fileName, unsigned int line)
> -{
> -	/* Log the timestamp, severity and file information. */
> -	timestamp_ = utils::clock::now();
> -
>  	std::ostringstream ossFileInfo;
>  	ossFileInfo << utils::basename(fileName) << ":" << line;
> -	fileInfo_ = ossFileInfo.str();
> +	fileInfo_ = std::move(ossFileInfo).str();

What is the benefit of moving ossFileInfo first and then calling ::str()
(which anyway returns a copy to the underlying string) ?

>  }
>
>  LogMessage::~LogMessage()
> --
> 2.48.1
>
>
Barnabás Pőcze Feb. 3, 2025, 6:15 p.m. UTC | #2
2025. február 3., hétfő 17:30 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> Hi Barnabás
> 
> On Thu, Jan 30, 2025 at 07:58:29PM +0000, Barnabás Pőcze wrote:
> > It is a short function that can be merged into the constructor with
> > essentially no change in observable behaviour, so do that.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/base/log.h |  2 --
> >  src/libcamera/base/log.cpp   | 13 +++----------
> >  2 files changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> > index b3050eedb..6d2c93019 100644
> > --- a/include/libcamera/base/log.h
> > +++ b/include/libcamera/base/log.h
> > @@ -75,8 +75,6 @@ public:
> >  private:
> >  	LIBCAMERA_DISABLE_COPY_AND_MOVE(LogMessage)
> >
> > -	void init(const char *fileName, unsigned int line);
> > -
> >  	std::ostringstream msgStream_;
> >  	const LogCategory &category_;
> >  	LogSeverity severity_;
> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > index 31053aee1..51a5cd470 100644
> > --- a/src/libcamera/base/log.cpp
> > +++ b/src/libcamera/base/log.cpp
> > @@ -875,19 +875,12 @@ const LogCategory &LogCategory::defaultCategory()
> >  LogMessage::LogMessage(const char *fileName, unsigned int line,
> >  		       const LogCategory &category, LogSeverity severity,
> >  		       const std::string &prefix)
> > -	: category_(category), severity_(severity), prefix_(prefix)
> > +	: category_(category), severity_(severity),
> > +	  timestamp_(utils::clock::now()), prefix_(prefix)
> >  {
> > -	init(fileName, line);
> > -}
> > -
> > -void LogMessage::init(const char *fileName, unsigned int line)
> > -{
> > -	/* Log the timestamp, severity and file information. */
> > -	timestamp_ = utils::clock::now();
> > -
> >  	std::ostringstream ossFileInfo;
> >  	ossFileInfo << utils::basename(fileName) << ":" << line;
> > -	fileInfo_ = ossFileInfo.str();
> > +	fileInfo_ = std::move(ossFileInfo).str();
> 
> What is the benefit of moving ossFileInfo first and then calling ::str()
> (which anyway returns a copy to the underlying string) ?

Oops, this is an unrelated change from my experimentation. In C++20 and later
one is able to move the string out of a stringstream without copying. Before
that there is no effect.


> 
> >  }
> >
> >  LogMessage::~LogMessage()
> > --
> > 2.48.1
> >
> >
>

Patch
diff mbox series

diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
index b3050eedb..6d2c93019 100644
--- a/include/libcamera/base/log.h
+++ b/include/libcamera/base/log.h
@@ -75,8 +75,6 @@  public:
 private:
 	LIBCAMERA_DISABLE_COPY_AND_MOVE(LogMessage)
 
-	void init(const char *fileName, unsigned int line);
-
 	std::ostringstream msgStream_;
 	const LogCategory &category_;
 	LogSeverity severity_;
diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
index 31053aee1..51a5cd470 100644
--- a/src/libcamera/base/log.cpp
+++ b/src/libcamera/base/log.cpp
@@ -875,19 +875,12 @@  const LogCategory &LogCategory::defaultCategory()
 LogMessage::LogMessage(const char *fileName, unsigned int line,
 		       const LogCategory &category, LogSeverity severity,
 		       const std::string &prefix)
-	: category_(category), severity_(severity), prefix_(prefix)
+	: category_(category), severity_(severity),
+	  timestamp_(utils::clock::now()), prefix_(prefix)
 {
-	init(fileName, line);
-}
-
-void LogMessage::init(const char *fileName, unsigned int line)
-{
-	/* Log the timestamp, severity and file information. */
-	timestamp_ = utils::clock::now();
-
 	std::ostringstream ossFileInfo;
 	ossFileInfo << utils::basename(fileName) << ":" << line;
-	fileInfo_ = ossFileInfo.str();
+	fileInfo_ = std::move(ossFileInfo).str();
 }
 
 LogMessage::~LogMessage()