[libcamera-devel] [PATCH] cam: convert to libfmt

[Laurent Pinchart]

Hi Tomi,

On Tue, May 10, 2022 at 04:47:56PM +0300, Tomi Valkeinen wrote:
> On 10/05/2022 16:13, Laurent Pinchart wrote:
> > On Tue, May 10, 2022 at 12:10:21PM +0300, Tomi Valkeinen wrote:
> >> On 10/05/2022 10:16, Tomi Valkeinen wrote:
> >>> This is just a conversation starter, not for merging. I really like
> >>> libfmt, and I really hate the C++ stream-based string formatting. libfmt
> >>> is the first library I add to any new C++ project I make.
> >>>
> >>> You can find more information about libfmt from:
> >>>
> >>> https://github.com/fmtlib/fmt
> >>> https://fmt.dev/latest/index.html
> >>>
> >>> This patch is just a crude conversion with ugly macros to showcase what
> >>> the formatting code might look like.
> >>
> >> As for the PR and EPR macros, those was just something to get forward
> >> with. I think fmt::print("") is fine, but fmt::print(stderr, "") is a
> >> bit long.
> >>
> >> Compared to cout, "fmt::print(" is actually shorter than "std::cout <<
> >> ", and without all those << and std::endl the lines are shorter.
> >>
> >> Not so with cerr and fmt::print(stderr, (although in many cases the
> >> total length would still be shorter) but I think it makes sense to have
> >> a macro/inline func for error prints, if only to make it more obvious
> >> that it's an error print.
> >>
> >> And, of course, libfmt could also be used for logging:
> >>
> >> LOG(Camera, Error) << "Camera in " << camera_state_names[currentState]
> >> 		   << " state trying " << from << "() requiring state "
> >> 		   << camera_state_names[state];
> >>
> >> to
> >>
> >> LOG(Camera, Error, "Camera in {} state trying {}() requiring state {}",
> >>       camera_state_names[currentState], from, camera_state_names[state]);
> > 
> > That's an interesting example, it clearly shows one of the main semantic
> > differences between the two options. std::format makes it easier to see
> > the whole message as it's stored in a single string, but the drawback is
> > that you have to go back and forth between the format string and the
> > arguments to figure out what is getting printed. std::ostream makes the
> > reading flow more natural in my opinion.
> 
> I think the bigger difference comes apparent when using more complex 
> formats than just plain {} (i.e. setting precision, width, alignment, etc).

Once you get used to how the precision and other parameters are
expressed with libfmt, it's indeed more concise, possibly easier to
read, and certainly easier to write.

> > I'm pretty sure my preference will vary based on the logging statement,
> > some will become more readable, others will get worse. That won't help
> > making a decision :-) It would be nice, however, to see how it would
> 
> Maybe there's a logging statement out there which is more readable with 
> streams, but I'm not aware of it ;).

I think the above example is more readable with streams :-)

> > affect logging, with a sample patch for the logger. It should ideally
> > also add a custom formatter for at least one of the geometry classes.
> 
> We already have that in this patch, as camera_session.cpp prints a Size. 
> But it uses the existing std::ostream formatter from geometry.cpp, so... 
> it's a bit boring =).
> 
> libfmt has its own formatter support, which is useful if you want to 
> pass formatting parameters to the formatter. Say, {:#x} and {:.2f} could 
> be used to print "(0x4, 0x7)" and "(4.00, 7.00)" for the same Size.

I meant using custom libfmt formatters, yes, the same way we have custom
operator<<() for the geometry classes. I'd like to see how that would
look like.

-- 
Regards,

Laurent Pinchart