Fix issue #2274. #2275

phprus · 2021-05-09T20:42:31Z

vitaut

Thanks for the PR.

vitaut · 2021-05-10T14:53:52Z

src/os.cc

+  }
+
+ public:
+  system_message(unsigned long error_code) : result_(0), message_(nullptr) {


Please make this ctor explicit.

vitaut · 2021-05-10T14:56:43Z

src/os.cc

+        reinterpret_cast<wchar_t*>(&message_), 0, nullptr);
+  }
+  ~system_message() { LocalFree(message_); }
+  unsigned long result() const FMT_NOEXCEPT { return result_; }


Let's replace this with explicit operator bool since callers don't need the actual result, just the success indicator.

vitaut · 2021-05-10T14:58:37Z

src/os.cc

+  wstring_view rtrim() const FMT_NOEXCEPT {
+    auto len = result_;
+    while (len != 0 && is_whitespace(message_[len - 1])) {
+      --len;
+    }
+    return wstring_view(message_, len);
+  }


Let's fold this into the constructor to always remove trailing whitespace.

fmt/src/os.cc

Lines 109 to 136 in 2a9b314

void detail::format_windows_error(detail::buffer<char>& out, int error_code,

const char* message) FMT_NOEXCEPT {

FMT_TRY {

wmemory_buffer buf;

buf.resize(inline_buffer_size);

for (;;) {

wchar_t* system_message = &buf[0];

int result = FormatMessageW(

FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, nullptr,

error_code, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), system_message,

static_cast<uint32_t>(buf.size()), nullptr);

if (result != 0) {

utf16_to_utf8 utf8_message;

if (utf8_message.convert(system_message) == ERROR_SUCCESS) {

format_to(buffer_appender<char>(out), "{}: {}", message,

utf8_message);

return;

}

break;

}

if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)

break; // Can't get error message, report error code instead.

buf.resize(buf.size() * 2);

}

}

FMT_CATCH(...) {}

format_error_code(out, error_code, message);

}

The original implementation of the detail::format_windows_error function does not remove trailing whitespace. For compatibility, I have separated the operator wstring_view() and the rtrim() function.

Modify code and change the behavior of a detail::format_windows_error function?

Modify code and change the behavior of a detail::format_windows_error function?

Sure.

vitaut · 2021-05-10T14:59:30Z

src/os.cc

+    system_message msg(error_code);
+    if (msg.result() != 0) {


With explicit operator bool proposed above this can be

if (system_message msg(error_code)) {

Visual Studio 2017:

F:\tmp\fmt\fmt\src\os.cc(141): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int F:\tmp\fmt\fmt\src\os.cc(141): error C2059: syntax error: '(' F:\tmp\fmt\fmt\src\os.cc(141): error C2143: syntax error: missing ';' before '{'

vitaut · 2021-05-10T15:03:40Z

src/os.cc

+    system_message msg(error_code);
+    if (msg.result() != 0) {


Same here:

if (system_message msg(error_code)) {

vitaut · 2021-05-10T15:06:02Z

test/os-test.cc

-  EXPECT_NE(result, 0);
+  if (result == 0) {
+    LocalFree(message);
+    return;
+  }


Please keep the old logic because we shouldn't skip test on failure.

Before commit 4b885c8 this test was successful if windows did not support the given error code.
On non-English version Windows 7 SP1 this test failed after commit 4b885c8.

FormatMessageW generates an error 15100. And test has comment:

// this error code is not available on all Windows platforms and // Windows SDKs, so do not fail the test if the error string cannot // be retrieved.

phprus · 2021-05-10T18:12:09Z

Unexpected CI Error: The operation was canceled on task "macos / build (Release) (pull_request)".
In my fork (https://github.com/phprus/fmt/tree/issue2274) all tests a passed.

src/os.cc

phprus mentioned this pull request May 9, 2021

Localization issue in vwindows_error #2274

Closed

vitaut requested changes May 10, 2021

View reviewed changes

phprus force-pushed the issue2274 branch from b6c5d40 to 3a2562e Compare May 10, 2021 17:55

phprus requested a review from vitaut May 10, 2021 18:12

vitaut reviewed May 10, 2021

View reviewed changes

src/os.cc Outdated Show resolved Hide resolved

Fix issue fmtlib#2274.

7e64a4e

phprus force-pushed the issue2274 branch from 3a2562e to 7e64a4e Compare May 10, 2021 19:22

phprus requested a review from vitaut May 10, 2021 20:28

vitaut merged commit 0036a1d into fmtlib:master May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #2274. #2275

Fix issue #2274. #2275

phprus commented May 9, 2021

vitaut left a comment

vitaut May 10, 2021

vitaut May 10, 2021

vitaut May 10, 2021

phprus May 10, 2021

vitaut May 10, 2021

vitaut May 10, 2021

phprus May 10, 2021

vitaut May 10, 2021

vitaut May 10, 2021

phprus May 10, 2021

phprus commented May 10, 2021

	void detail::format_windows_error(detail::buffer<char>& out, int error_code,
	const char* message) FMT_NOEXCEPT {
	FMT_TRY {
	wmemory_buffer buf;
	buf.resize(inline_buffer_size);
	for (;;) {
	wchar_t* system_message = &buf[0];
	int result = FormatMessageW(
	FORMAT_MESSAGE_FROM_SYSTEM \| FORMAT_MESSAGE_IGNORE_INSERTS, nullptr,
	error_code, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), system_message,
	static_cast<uint32_t>(buf.size()), nullptr);
	if (result != 0) {
	utf16_to_utf8 utf8_message;
	if (utf8_message.convert(system_message) == ERROR_SUCCESS) {
	format_to(buffer_appender<char>(out), "{}: {}", message,
	utf8_message);
	return;
	}
	break;
	}
	if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
	break; // Can't get error message, report error code instead.
	buf.resize(buf.size() * 2);
	}
	}
	FMT_CATCH(...) {}
	format_error_code(out, error_code, message);
	}

Fix issue #2274. #2275

Fix issue #2274. #2275

Conversation

phprus commented May 9, 2021

vitaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phprus commented May 10, 2021