Skip to content

Log filesystem path instead of string for C++20 compliance#26745

Open
JanSellner wants to merge 2 commits intomicrosoft:mainfrom
JanSellner:logging_bug
Open

Log filesystem path instead of string for C++20 compliance#26745
JanSellner wants to merge 2 commits intomicrosoft:mainfrom
JanSellner:logging_bug

Conversation

@JanSellner
Copy link

Description

Log the std::filesystem::path object instead of the ORTCHAR_T string to avoid C++20 compliance issues.

Motivation and Context

ORTCHAR_T is a wchar_t on Windows and it is not allowed to pass that to std::basic_ostream.

Example

(example.cpp)

#include <iostream>
#include <filesystem>

int main() {
    const wchar_t* s = L"my/path";
    std::filesystem::path path(s);

    std::cout << s << std::endl;

    return 0;
}

Output on latest MSVC with /std:c++20:

<source>(8): error C2280: 'std::basic_ostream<char,std::char_traits<char>> &std::operator <<<std::char_traits<char>>(std::basic_ostream<char,std::char_traits<char>> &,const wchar_t *)': attempting to reference a deleted function
Z:\compilers\msvc\14.44.35207-14.44.35219.0\include\__msvc_ostream.hpp(972): note: see declaration of 'std::operator <<'
<source>(8): error C2088: built-in operator '<<' cannot be applied to an operand of type 'std::ostream'

@JanSellner
Copy link
Author

@skottmckay Anything I can do to move forward with this (small) PR?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a C++20 compliance issue in the LoadPluginOrProviderBridge function where a wchar_t* string (represented as ORTCHAR_T* on Windows) was being logged to std::ostream, which is not allowed in C++20. The fix changes the logging statement to use the std::filesystem::path object instead.

Changes:

  • Updated logging statement to use resolved_library_path (std::filesystem::path) instead of library_path (ORTCHAR_T*) for C++20 compliance

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@eserscor
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@skottmckay
Copy link
Contributor

Can you rebase your branch to pickup the latest main? We're not able to trigger the CIs which are required before checkin and that might be the cause as the problem only seems to affect your PR.

image

@JanSellner
Copy link
Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 26745 in repo microsoft/onnxruntime

@JanSellner
Copy link
Author

Hmm, worth a try :P
Branch is up-to-date now.

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.

3 participants