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

Feature request: Slim source location without function name #1337

Closed
jonwis opened this issue Jul 28, 2023 · 18 comments · Fixed by #1379
Closed

Feature request: Slim source location without function name #1337

jonwis opened this issue Jul 28, 2023 · 18 comments · Fixed by #1379
Assignees

Comments

@jonwis
Copy link
Member

jonwis commented Jul 28, 2023

Version

2.0.220131.2

Summary

The std::source_location is super useful, but very very noisy in terms of binary size. Turning it on for a certain OS binary increased its size from 1500kb to 2100kb, or about 40%. While the actual strings are likely to be optimized away into a cold section of the binary, the additional DLL size tradeoff is not appropriate. WIL, for instance, uses just __FILE__ and __LINE__ as the markers, and we've been very successful debugging with those.

There is no means to tell std::source_location to capture only file & line information.

I propose an option for winrt::source_location which uses a technique like

Reproducible example

#include <algorithm>
#include <source_location>

namespace winrt
{
    namespace details
    {
        struct source_location
        {
            const char* file;
            uint32_t line;
        };

        template<size_t length> struct string_literal {
            constexpr string_literal(const char(&str)[length]) {
                std::copy_n(str, length, value);
            }
            char value[length];
        };

        template<string_literal file, uint32_t line> struct source_loc
        {
            inline static constexpr source_location loc{file.value, line};
        };

        constexpr source_location from_std_location(std::source_location src)
        {
            source_location loc;
            loc.file = src.file_name();
            loc.line = src.line();
            return loc;
        }
    }
}

#define SOURCE_LOCATION_CURRENT winrt::details::source_loc<__FILE__, __LINE__>::loc
#define SOURCE_LOCATION_CURRENT_STD winrt::details::from_std_location(std::source_location::current())

Expected behavior

No response

Actual behavior

No response

Additional comments

No response

@kennykerr
Copy link
Collaborator

I'll defer to @dmachaj as this was his feature.

@dmachaj
Copy link
Contributor

dmachaj commented Jul 28, 2023

We have an email thread going on this topic. I think it would make sense for DEBUG to include full std::source_location and non-DEBUG to have a more minimal source_location that excludes the function name (which is the biggest binary impact).

@sylveon
Copy link
Contributor

sylveon commented Jul 28, 2023

Might not be a terrible idea to fill a feature request at https://github.com/microsoft/STL to give users some macro/escape hatch to remove function name - I know of a few other people not wanting to use it specifically because it would embed function names into their binaries.

@frederick-vs-ja
Copy link

frederick-vs-ja commented Jul 29, 2023

Possible implementation:

struct slim_source_location {
    [[nodiscard]] static consteval slim_source_location current(const uint_least32_t _line_ = __builtin_LINE(),
        const uint_least32_t _col_ = __builtin_COLUMN(), const char* const _file_ = __builtin_FILE()) noexcept {
        slim_source_location res{};
        res.line_   = _line_;
        res.column_ = _col_;
        res.file_   = _file_;
        return res;
    }

    [[nodiscard]] constexpr slim_source_location() noexcept = default;

    [[nodiscard]] constexpr uint_least32_t line() const noexcept {
        return line_;
    }
    [[nodiscard]] constexpr uint_least32_t column() const noexcept {
        return column_;
    }
    [[nodiscard]] constexpr const char* file_name() const noexcept {
        return file_;
    }

private:
    uint_least32_t line_{};
    uint_least32_t column_{};
    const char* file_ = "";
};

@github-actions
Copy link

github-actions bot commented Aug 9, 2023

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions
Copy link

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions
Copy link

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions
Copy link

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions
Copy link

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Copy link

github-actions bot commented Nov 6, 2023

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Copy link

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Copy link

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Copy link

github-actions bot commented Dec 9, 2023

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Copy link

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@dmachaj
Copy link
Contributor

dmachaj commented Dec 19, 2023

@kennykerr the bot seems a bit overzealous. I don't see how attaching a PR to an issue counts as "no activity".

@kennykerr
Copy link
Collaborator

It's a standard github action. There may be an update or config option you can tweak.

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 a pull request may close this issue.

5 participants