[tests] add log level to string unit tests

[xmkg]

This MP adds log level to string unit test case to ensure all cases are covered.

The MP also includes the commits from #3944 to resolve cyclic dependency between the two branches causing a CI failure

[giuliazanchi] Fixed link to Multipass Discourse landing page

MULTI-1831

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base (ec295dc) to head (869a5c7).
Report is 3472 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3943   +/-   ##
=======================================
  Coverage   89.11%   89.11%           
=======================================
  Files         255      255           
  Lines       14603    14603           
=======================================
  Hits        13014    13014           
  Misses       1589     1589           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[xmkg]

Cherry-picked the commits from #3944 to resolve the cyclic CI failure between the two branches.

[Sploder12]

LGTM!

Not really relating to this PR but mpl::as_string returning CString is interesting, std::string_view seems more appropriate.

[georgeliao]

It looks good to me as well. CString is a in-house utility class which should be removed.

[xmkg]

LGTM!

Not really relating to this PR but mpl::as_string returning CString is interesting, std::string_view seems more appropriate.

Yeah, I'm not fond of CString as well. The only reason it works is the expression temporary lifetime extension:

void foo(CString f){ }

foo(fmt::format("test")); 
// fmt::format returns a std::string, which has a temporary lifetime. 
// CString has a const std::string& constructor which grabs the return value 
// of .c_str() to assign it to a private const char* member.
// The lifetime of the temporary is extended just because of the expression `foo(...)`.
// The following is UB:

CString x = fmt::format("test");
foo(x);

Ideally, the log function should take perfectly forwarded parameters and bind them to something that is not temporary.

[ricab]

+1 on the CString stuff, but I would prefer avoiding string_views as well. They're unfortunately pretty broken/dangerous in C++. For example, they suffer from the exact same problem that @xmkg pointed out.

[xmkg]

+1 on the CString stuff, but I would prefer avoiding string_views as well. They're unfortunately pretty broken/dangerous in C++. For example, they suffer from the exact same problem that @xmkg pointed out.

At least, you know that std::string_view is not supposed to own memory, hence it's a little bit clear (compared to CString) that you shouldn't bind a temporary to a string_view. What I don't like about string_view is it's not guaranteed to be NUL terminated, and that does not play nicely with the API that relies on the presence of '\0'. It's not uncommon that I see std::string_view.c_str().data() being passed around for a format parameter to "%s", for example, where the correct way to format a string view is to use %.*s, which might not be available as a format specifier depending on the platform.

[Sploder12]

It's not uncommon that I see std::string_view.c_str() being passed around for a format parameter to "%s", for example, where the correct way to format a string view is to use %.*s, which might not be available as a format specifier depending on the platform.

std::string_view doesn't have c_str for that very reason, but I've seen data used for that purpose. Perfect forwarding to the log function would be nice but then it wouldn't be able to be polymorphic. I'd argue the "correct" way to format a string view is to not use a C API to format it.

[xmkg]

std::string_view doesn't have c_str for that very reason

ah yes, I mean data().

I'd argue the "correct" way to format a string view is to not use a C API to format it

Well, unfortunately, the format specifiers are language-agnostic. I'd rather prefer an explicit length specifier %.*s and not have a %s specifier at all. The whole NUL Terminator thing always felt like a hack to me. I mean, if you know the size of the data you have, why do you need a sentinel to indicate its end in the first place? Remnants of old times and old concerns.

[xmkg]

Perfect forwarding to the log function would be nice but then it wouldn't be able to be polymorphic

Do we need it to be, though? The perfect forwarding function would act as an interim step that produces the final formatted string and calls the concrete, polymorphic log() function, which would do the actual logging. It should be good enough for mocking purposes.

[xmkg]

The perfect forwarding function would act as an interim step that produces the final formatted string

We could also embed the fmt::format() to the interim function, so the log calls would only specify a format string and arguments, if any:

log(level::debug, "{} {}", arg1, arg2);

[Sploder12]

Do we need it to be, though? The perfect forwarding function would act as an interim step that produces the final formatted string and call the concrete, polymorphic log() function, which would do the actual logging. It should be good enough for mocking purposes.

Good point, I think that'd work really well! +1 from me for that approach!

[xmkg]

Good point, I think that'd work really well! +1 from me for that approach!

I might give that a stab on the next pulse -- I need to fiddle with the log() API to add Unicode support for Windows anyway.

[xmkg]

Good point, I think that'd work really well! +1 from me for that approach!

I might give that a stab on the next pulse -- I need to fiddle with the log() API to add Unicode support for Windows anyway.

#3957