Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added std::type_info formatter #3978

Merged
merged 1 commit into from
May 28, 2024
Merged

Added std::type_info formatter #3978

merged 1 commit into from
May 28, 2024

Conversation

matt77hias
Copy link
Contributor

  • Added std::type_info formatter;
  • Reused std::type_info formatter in std::exception formatters;
  • Updated MSVC std::type_info outputting to exclude all class, struct and enum occurences.

@matt77hias
Copy link
Contributor Author

Ok still WIP in order to support wchar_t. Ignore for now, but please leave it open so I can borrow the CI.

@matt77hias matt77hias force-pushed the type_info branch 4 times, most recently from 75cdef1 to ce222b5 Compare May 25, 2024 18:29
@matt77hias
Copy link
Contributor Author

Update: removed union occurrences as well for MSVC

@matt77hias matt77hias force-pushed the type_info branch 2 times, most recently from cb58b68 to 7c74917 Compare May 25, 2024 18:39
@phprus
Copy link
Contributor

phprus commented May 25, 2024

I think formatting std::type_info should not change the output in MSVC. Removing all substrings may distort the result.

Before your changes, std::exception formatting used minimal additional memory allocations (only for Itanium ABI).. I think it makes sense to keep this behavior.

@matt77hias
Copy link
Contributor Author

matt77hias commented May 25, 2024

I think formatting std::type_info should not change the output in MSVC. Removing all substrings may distort the result.

Note that prior to my changes, the class or struct prefix was already removed. Removing just these prefixes is somewhat arbitrary:

  • It does not include enums and unions (which admittedly wasn't a concern for classes deriving from std::exception);
  • It does not consider type names inside templates (a template class could derive from std::exception). For example, typeid(std::vector<Foo>) would be formatted as std::vector<class Foo,...> (ignoring the allocator for easier discussion), while typeid(std::vector<Foo>::value_type) would be formatted as just Foo.

Before your changes, std::exception formatting used minimal additional memory allocations (only for Itanium ABI).. I think it makes sense to keep this behavior.

I think we can output the characters one by one while jumping over the occurrences instead of using a std::basic_string.

I do wonder whether I could erronously strip part of the actual identifier, instead of language keywords. All of the patterns do contain a space character that should prevent this. MSVC does not seem to use spaces before and after ,, < and >. But that is all speculative of me.

@phprus
Copy link
Contributor

phprus commented May 26, 2024

Suggested changes

Create function:

namespace detail {

template <typename Char, typename OutputIt>
auto write_demangled_name(OutputIt out,
                          std::type_info& ti) -> OutputIt {
#  ifdef FMT_HAS_ABI_CXA_DEMANGLE
    int status = 0;
    std::size_t size = 0;
    std::unique_ptr<char, void (*)(void*)> demangled_name_ptr(
        abi::__cxa_demangle(ti.name(), nullptr, &size, &status), &std::free);

    string_view demangled_name_view;
    if (demangled_name_ptr) {
      demangled_name_view = demangled_name_ptr.get();

      // Normalization of stdlib inline namespace names.
      // libc++ inline namespaces.
      //  std::__1::*       -> std::*
      //  std::__1::__fs::* -> std::*
      // libstdc++ inline namespaces.
      //  std::__cxx11::*             -> std::*
      //  std::filesystem::__cxx11::* -> std::filesystem::*
      if (demangled_name_view.starts_with("std::")) {
        char* begin = demangled_name_ptr.get();
        char* to = begin + 5;  // std::
        for (char *from = to, *end = begin + demangled_name_view.size();
             from < end;) {
          // This is safe, because demangled_name is NUL-terminated.
          if (from[0] == '_' && from[1] == '_') {
            char* next = from + 1;
            while (next < end && *next != ':') next++;
            if (next[0] == ':' && next[1] == ':') {
              from = next + 2;
              continue;
            }
          }
          *to++ = *from++;
        }
        demangled_name_view = {begin, detail::to_unsigned(to - begin)};
      }
    } else {
      demangled_name_view = string_view(ti.name());
    }
    return detail::write_bytes<Char>(out, demangled_name_view);
#  elif FMT_MSC_VERSION
    std::string demangled_name(ti.name());
    details::remove_all_substrings(demangled_name, "class ");
    details::remove_all_substrings(demangled_name, "enum ");
    details::remove_all_substrings(demangled_name, "struct ");
    details::remove_all_substrings(demangled_name, "union ");
    return detail::write_bytes<Char>(out, demangled_name);
#  else
    return detail::write_bytes<Char>(out, string_view(ti.name()));
#  endif
}

}

and reuse it in formatters.

We don't need to use detail::string_literal and std::basic_string<Char> since std::type_info::name() always returns const char *

@matt77hias
Copy link
Contributor Author

matt77hias commented May 26, 2024

  • Added write_demangled_name function as suggested (but for const std::type_info&);
  • Removed std::string for MSVC;
  • Excluded white space characters as well;
    • E.g., DynamicVector<int,GlobalMemoryAllocator<1>> instead of DynamicVector<int,GlobalMemoryAllocator<1> >;

Out of scope improvements:

  • Implementation could still benefit from basic_string_view::size_type, basic_string_view::front, basic_string_view::substr (which do not exist as of writing);
  • An optional space can be added after < or before > to match style preferences.

@phprus
Copy link
Contributor

phprus commented May 26, 2024

LGTM

@matt77hias matt77hias force-pushed the type_info branch 2 times, most recently from 55387ae to 3bf8a0b Compare May 26, 2024 15:30
Comment on lines 538 to 539
#else
out = detail::write_bytes<Char>(out, string_view("std::exception"));
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry :(
It was my mistake.
#else code will never execute if FMT_USE_RTTI is disabled, because with_typename_ = FMT_USE_RTTI != 0; in parse() function.

More correct variant is remove this #else block and move #endif after return detail::write_bytes<Char>(out, string_view(ex.what()));
This was before PR.

I'm sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be more clear to do something like the below instead:

#if FMT_USE_RTTI
    if (with_typename_) {
      out = detail::write_demangled_name<Char>(out, typeid(ex));
      *out++ = ':';
      *out++ = ' ';
    }
#endif
    return detail::write_bytes<Char>(out, string_view(ex.what()));

?

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good but please a test case for the new formatter.

@matt77hias
Copy link
Contributor Author

matt77hias commented May 27, 2024

It is actually implicitly tested already via the exception formatter.
But I can add another one. I'll reuse one of the exceptions because even something as simple as int outputs something different on Clang and GCC.

Comment on lines +303 to +307
TEST(std_test, type_info) {
EXPECT_EQ(fmt::format("{}", typeid(std::runtime_error)),
"std::runtime_error");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be wrapped in FMT_USE_RTTI check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

EXPECT_EQ("std::runtime_error: Test Exception", fmt::format("{:t}", ex));
EXPECT_THAT(fmt::format("{:t}", ex), StartsWith("std::system_error: "));
EXPECT_THAT(fmt::format("{:t}", ex),
                StartsWith("std::filesystem::filesystem_error: "));

None of these tests would succeed without FMT_USE_RTTI, which is not used as guard in std-test.cc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a bug.

* Added std::type_info formatter;
* Reused std::type_info formatter in std::exception formatters;
* Updated MSVC std::type_info outputting to exclude all class, struct and enum occurences.
@vitaut vitaut merged commit 728f9bc into fmtlib:master May 28, 2024
43 checks passed
@matt77hias matt77hias deleted the type_info branch May 28, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants