Skip to content

Enable string-views for use with Tracelogging #589

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.

Sign up for GitHub

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

Draft
jonwis wants to merge 2 commits into
base: master
Choose a base branch
from
+155 −16
Draft

Enable string-views for use with Tracelogging #589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ed34ae
Add support for tracelogging of string types
jonwis May 25, 2025
6ab48c8
Merge remote-tracking branch 'origin/master' into user/jonwis/tracelo…
jonwis Dec 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions CMakePresets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"installDir": "${sourceDir}/build/install/${presetName}",
"toolchainFile": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake",
"cacheVariables": {
"WIL_ENABLE_ASAN": true,
"CMAKE_CONFIGURATION_TYPES": "Debug;RelWithDebInfo;Release;MinSizeRel",
"CMAKE_CXX_COMPILER": "cl",
"CMAKE_C_COMPILER": "cl"
Expand All @@ -29,29 +30,33 @@
}
},
"cacheVariables": {
"WIL_ENABLE_ASAN": false,
"CMAKE_CXX_COMPILER": "clang-cl",
"CMAKE_C_COMPILER": "clang-cl"
}
},
{
"name": "clang-release",
"inherits": "clang",
"hidden": false,
"cacheVariables": {
"WIL_ENABLE_ASAN": true,
"WIL_ENABLE_UBSAN": true
}
}
],
"buildPresets": [
{
"name": "msvc-debug",
"displayName": "MSVC Debug",
"configurePreset": "msvc",
"configuration": "Debug",
"cacheVariables": {
"WIL_ENABLE_ASAN": true
}
"configuration": "Debug"
},
{
"name": "msvc-release",
"displayName": "MSVC Release (debuggable)",
"configurePreset": "msvc",
"configuration": "RelWithDebInfo",
"cacheVariables": {
"WIL_ENABLE_ASAN": true
}
"configuration": "RelWithDebInfo"
},
{
"name": "clang-debug",
Expand All @@ -62,12 +67,8 @@
{
"name": "clang-release",
"displayName": "clang Release (debuggable)",
"configurePreset": "clang",
"configuration": "RelWithDebInfo",
"cacheVariables": {
"WIL_ENABLE_ASAN": true,
"WIL_ENABLE_UBSAN": true
}
"configurePreset": "clang-release",
"configuration": "RelWithDebInfo"
}
],
"testPresets": [
Expand Down
71 changes: 71 additions & 0 deletions include/wil/stl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <string>
#include <vector>
#include <utility>
#include <TraceLoggingProvider.h>

@idigdoug idigdoug Dec 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Technically this is illegally reaching into internal implementation details of TraceLoggingProvider.h.

Probably the right thing to do is:

  1. Modify TraceLoggingProvider.h to expose enough "supported" machinery to support some C++ constructs.
  2. Modify WIL to use the supported mechanism if present, or to poly-fill the machinery if not present. (The poly-fill will only be used on old versions of TraceLoggingProvider.h, so future changes to TraceLoggingProvider.h internals will not break the poly-fill.

#if (__WI_LIBCPP_STD_VER >= 17) && WI_HAS_INCLUDE(<string_view>, 1) // Assume present if C++17
#include <string_view>
#endif
Expand Down Expand Up @@ -266,6 +267,76 @@ struct std::formatter<wil::basic_zstring_view<TChar>, TChar> : std::formatter<st
};
#endif

#if __cpp_lib_string_view >= 201606L

template <typename TContainer>
struct _tlgWrapBufferStlContainer

@idigdoug idigdoug Dec 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The _tlg prefix is used to signal "TraceLoggingProvider.h-internal machinery". This prefix should not be used for "WIL-internal machinery", even if it is "WIL-internal TraceLogging-related" machinery.

{
static const unsigned DataDescCount = 2;

TContainer const& view;

__forceinline explicit _tlgWrapBufferStlContainer(_In_ TContainer const& ref) : view(ref)

@keith-horton keith-horton Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does this need our special __forceinline keyword? can we just use the standard 'inline' keyword?

@keith-horton keith-horton Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

as this is a template parameter, can we use a 'Universal Reference' with std::forward?

@idigdoug idigdoug Jan 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't think that achieves anything here. Universal reference is so that you can accept both const and non-const references, or so you can accept both non-const-rvalue and non-const-lvalue references. We only need to be able to accept a const reference, so no fancy universal reference is needed.

@keith-horton keith-horton Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that's because moves don't buy us anything for these templated types vs. making copies?

@idigdoug idigdoug Jan 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are no copies. It's a const ref all the way down.

@keith-horton keith-horton Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oops. yep. sorry, missed that!

@keith-horton keith-horton Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Jon - feel free to close this. I can't seem to resolve/close it myself :(

{
}

__forceinline void* Fill(_Out_writes_(DataDescCount) EVENT_DATA_DESCRIPTOR* pDesc) const
{
EventDataDescCreate(&pDesc[0], &pDesc[1].Size, 2);
EventDataDescCreate(&pDesc[1], view.data(), static_cast<UINT16>(view.size() * sizeof(*view.data())));
return pDesc;
}
};

template <typename TChar>
struct _tlgTypeMapStlString

@idigdoug idigdoug Dec 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should not use _tlg prefix for this.

@idigdoug idigdoug Dec 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should probably have a specialization for char8_t that sets OutType to UTF8.

{
typedef UINT8 _tlgTypeType0;
typedef UINT16 _tlgTypeType1;
static bool const _tlgIsSimple = false;

@keith-horton keith-horton Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can these be constexpr?

@idigdoug idigdoug Jan 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There isn't really any significant difference between class scope static bool const and static bool constexpr. A constexpr would allow more complex variables to be used here (const static only allows trivial scalars to be used without a corresponding extern), but we only need a bool.

@keith-horton keith-horton Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sure. I tend to make everything constexpr that can be supported, as I don't assume what the compiler can optimize / change where it puts such things - so I make it the rule, not the exception :)

it's clearly not a bit deal to go either way to me.

static TlgIn_t const _tlgViewIn = wistd::is_same_v<TChar, char> ? TlgInCOUNTEDANSISTRING : TlgInCOUNTEDSTRING;

@idigdoug idigdoug Dec 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should probably be based on sizeof(TChar) so it can do something reasonable with char8_t and char16_t. (And it should error-out for sizes other than 1 and 2.)

static _tlgTypeType0 const _tlgType0 = _tlgViewIn | 0x0000;
static _tlgTypeType1 const _tlgType1 = _tlgViewIn | 0x8080;
};

template <typename TChar, typename TTraits>
struct _tlgTypeMapBase<std::basic_string_view<TChar, TTraits>> : _tlgTypeMapStlString<TChar>
{
};

template <typename TChar, typename TTraits>
TLG_INLINE auto _tlg_CALL _tlgWrapAuto(std::basic_string_view<TChar, TTraits> const& value)

@idigdoug idigdoug Dec 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't use _tlg_CALL here -- it's an unnecessary use of TraceLoggingProvider.h internal machinery. Instead, use WINAPI or STDCALL or similar.

@keith-horton keith-horton Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why would we need a calling convention for this template function? is it passed as a function pointer somewhere?

@idigdoug idigdoug Jan 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In practice, it's probably not particularly significant. However, it is possible for two LIBs to use different default calling conventions and both include the header. In that case, the linker will see two distinct inline functions, and that can be confusing or sub-optimal. I just find it to be best practice to put a calling convention on any non-member function declaration in a header.

@keith-horton keith-horton Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok. I don't think we put calling conventions on standalone functions anywhere else in wil :)

@idigdoug idigdoug Dec 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't need TLG_INLINE here -- it's an unnecessary obfuscation. Just use inline.

@keith-horton keith-horton Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we probably don't need TLG_INLINE either.

{
return _tlgWrapBufferStlContainer<std::basic_string_view<TChar, TTraits>>(value);
}

template <typename TChar, typename TTraits>
struct _tlgTypeMapBase<std::basic_string<TChar, TTraits>> : _tlgTypeMapStlString<TChar>
{
};

template <typename TChar, typename TTraits>
TLG_INLINE auto _tlg_CALL _tlgWrapAuto(std::basic_string<TChar, TTraits> const& value)
{
return _tlgWrapBufferStlContainer<std::basic_string<TChar, TTraits>>(value);
}

template <typename TChar>
struct _tlgTypeMapBase<wil::basic_zstring_view<TChar>> : _tlgTypeMapStlString<TChar>
{
};

template <typename TChar>
TLG_INLINE auto _tlg_CALL _tlgWrapAuto(wil::basic_zstring_view<TChar> const& value)
{
return _tlgWrapBufferStlContainer<wil::basic_zstring_view<TChar>>(value);
}

#define TraceLoggingStringView(pValue, ...) _tlgArgAuto(static_cast<std::string_view>(pValue), __VA_ARGS__)
#define TraceLoggingWideStringView(pValue, ...) _tlgArgAuto(static_cast<std::wstring_view>(pValue), __VA_ARGS__)

@idigdoug idigdoug Dec 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The parameter to this macro should not be a pointer, so probably shouldn't be named pValue.


#endif // __cpp_lib_string_view >= 201606L

#endif // WIL_ENABLE_EXCEPTIONS

#endif // __WIL_STL_INCLUDED
8 changes: 6 additions & 2 deletions scripts/runtests.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ if not exist %BUILD_DIR% ( goto :eof )
:: MSVC does not currently ship a version of the x64 ASan DLL that can be run on an arm64 host. MSVC _does_ ship a lib
:: so we _can_ build the ASan tests, which is at least something. For now we're going to handle this limitation here and
:: avoid running the ASan test in this specific scenario
set RUN_ASAN_TEST=1
if not %RUN_ASAN_TEST%=='' set RUN_ASAN_TEST=1

@dunhor dunhor Jan 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume your intent is to initialize if not set, in which case the not seems out of place. Additionally, you probably want to surround with quotes so that once variable substitution takes place, you don't have something empty on the left hand side, which normally causes errors and is probably why CI is failing. E.g.

Suggested change
if not %RUN_ASAN_TEST%=='' set RUN_ASAN_TEST=1
if "%RUN_ASAN_TEST%"=="" set RUN_ASAN_TEST=1

if %PROCESSOR_ARCHITECTURE%==ARM64 (
if %2==x64 set RUN_ASAN_TEST=0
)
Expand Down Expand Up @@ -69,11 +69,15 @@ if %ERRORLEVEL% NEQ 0 ( goto :execute_tests_done )

:execute_tests_done
set EXIT_CODE=%ERRORLEVEL%
if %EXIT_CODE% NEQ 0 (
echo Tests failed in %BUILD_DIR% with exit code %EXIT_CODE%
goto :eof
)
popd
exit /B %EXIT_CODE%

:execute_test
if not exist tests\%1\%2 ( goto :eof )
echo Running %1 tests...
echo Running tests\%1\%2 %TEST_ARGS%
tests\%1\%2 %TEST_ARGS%
goto :eof
63 changes: 63 additions & 0 deletions tests/StlTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,4 +244,67 @@ TEST_CASE("StlTests::TestZWStringView", "[stl][zstring_view]")
REQUIRE(wil::zwstring_view(fake_path) == L"hello");
}

/* bd2bf191-ac1a-4732-b563-bb3e3006f617 */
TRACELOGGING_DEFINE_PROVIDER( // defines g_hProvider
g_tlgProvider, // Name of the provider variable
"MyProvider", // Human-readable name of the provider
(0xbd2bf191, 0xac1a, 0x4732, 0xb5, 0x63, 0xbb, 0x3e, 0x30, 0x06, 0xf6, 0x17)); // Provider GUID

TEST_CASE("TraceLogging for Views", "[tracelogging]")
{
#ifdef WIL_ENABLE_EXCEPTIONS
std::string_view sv = "kittens";
std::wstring_view wsv = L"kittens";
std::string s = "puppies";
std::wstring ws = L"puppies";
wil::zstring_view zv = "hamsters";
wil::zwstring_view zwv = L"hamsters";

std::string_view const& rsv = sv;
std::wstring_view const& rwsv = wsv;
wil::zstring_view const& rzv = zv;
wil::zwstring_view const& rzwv = zwv;

TraceLoggingRegister(g_tlgProvider);

TraceLoggingWrite(
g_tlgProvider,
"Hello",
TraceLoggingStringView(sv, "sv"),
TraceLoggingWideStringView(wsv, "wsv"),
TraceLoggingStringView(s, "s"),
TraceLoggingWideStringView(ws, "ws"),
TraceLoggingStringView(zv, "zv"),
TraceLoggingWideStringView(zwv, "zwv"));

TraceLoggingWrite(
g_tlgProvider,
"There",
TraceLoggingValue(sv, "sv"),
TraceLoggingValue(wsv, "wsv"),
TraceLoggingValue(s, "s"),
TraceLoggingValue(ws, "ws"),
TraceLoggingValue(zv, "zv"),
TraceLoggingValue(zwv, "zwv"));

TraceLoggingWrite(
g_tlgProvider,
"HelloRefs",
TraceLoggingStringView(rsv, "sv"),
TraceLoggingWideStringView(rwsv, "wsv"),
TraceLoggingStringView(rzv, "zv"),
TraceLoggingWideStringView(rzwv, "zwv"));

TraceLoggingWrite(
g_tlgProvider,
"ThereRefs",
TraceLoggingValue(rsv, "sv"),
TraceLoggingValue(rwsv, "wsv"),
TraceLoggingValue(rzv, "s"),
TraceLoggingValue(rzwv, "ws"));

TraceLoggingUnregister(g_tlgProvider);
#endif
}

#endif
Loading