fmt icon indicating copy to clipboard operation
fmt copied to clipboard

Non-arithmetic format_as is broken with fmt/core.h

Open jeremy-rifkin opened this issue 1 year ago • 4 comments

#3959 suggests that format_as should be supported with just fmt/core.h. I have hit a very surprising behavior where format_as returning a non-arithmetic type is completely broken unless including fmt/format.h: https://godbolt.org/z/344bznojf.

I see fmt/core.h is now just an alias for fmt/format.h on master so this should not reproduce currently, however, I want to flag this behavior in case it is unintentional and in case there are 10.x patch releases before the current core.h change is released.

jeremy-rifkin avatar Jun 13 '24 15:06 jeremy-rifkin

Closing since it doesn't reproduce on trunk and there are no plans for more 10.x releases but thanks for reporting.

vitaut avatar Jun 13 '24 19:06 vitaut

This seems to still be the case on trunk with base.h instead of core.h. format_as(Thing) -> int works with just base.h, but format_as(Thing) -> fmt::string_view or format_as(Thing) -> const char* requires format.h.

example: https://godbolt.org/z/5f75q8hh3

jagoly avatar Jun 28 '24 16:06 jagoly

It’s possible CE’s trunk version of fmt is outdated

jeremy-rifkin avatar Jun 28 '24 16:06 jeremy-rifkin

Thanks for the repro, @jagoly, now I see the problem. The fix would be to move this formatter https://github.com/fmtlib/fmt/blob/b61c8c3d23b7e6fdf9d44593877dba1c8a291be1/include/fmt/format.h#L3970-L3971 to fmt/base.h. A PR would be welcome.

vitaut avatar Jun 28 '24 16:06 vitaut

It seems simply moving that over isn't enough, that just gives errors in base::format(val, ctx); because native_formatter::format is only declared in base.h, with the definition in format.h.

I believe the correct solution is to tweak https://github.com/fmtlib/fmt/blob/c4f6fa71357b223b0ab8ac29577c6228fde8853d/include/fmt/base.h#L1502 so that it can map any types where format_as returns a natively supported type. I don't see why mapping to a string view would be an issue, lifetime extension should keep the original object around until the end of the format call anyway? Maybe it has issues with dynamic argument stores?

I've tried to get this working, though when adding tests to test-base.cc it seems that its_a_trap breaks when adding any format_as overloads to the file, whether they return a mappable type or not. I could work around this by adding a new test file just for format_as, but it's probably best if someone with a bit more familiarity looks at it.

jagoly avatar Jul 01 '24 23:07 jagoly

It seems simply moving that over isn't enough ...

You are right. Unfortunately doing this for user-defined types would be unsafe and lifetime extension won't apply.

In any case, since includes in fmt/format.h have been optimized there is little value in trying to optimize this so I'm closing the issue.

vitaut avatar Jul 03 '24 20:07 vitaut