From 7c2cd81edb5076f34bc067396412191be216f59c Mon Sep 17 00:00:00 2001 From: asynts Date: Tue, 6 Oct 2020 13:32:00 +0200 Subject: [PATCH] AK+Format: Exclude prefix from width calculation. When we write the format specifier '{:#08x}' we are asking for eight significant digits, zero padding and the prefix '0x'. However, previously we got only six significant digits because the prefix counted towards the width. (The number '8' here is the total width and not the number of significant digits.) Both fmtlib and printf shared this behaviour. However, I am introducing a special case here because when we do zero padding we really only care about the digits and not the width. Notice that zero padding is a special case anyways, because zero padding goes after the prefix as opposed to any other padding which goes before it. --- AK/Format.cpp | 30 +++++++++++++++------- AK/Format.h | 5 ++-- AK/Tests/TestFormat.cpp | 4 ++- Applications/HexEditor/HexEditor.cpp | 4 +-- Applications/HexEditor/HexEditorWidget.cpp | 2 +- 5 files changed, 30 insertions(+), 15 deletions(-) diff --git a/AK/Format.cpp b/AK/Format.cpp index 772488537f..9f8138ceb5 100644 --- a/AK/Format.cpp +++ b/AK/Format.cpp @@ -281,18 +281,30 @@ void FormatBuilder::put_u64( SignMode sign_mode, bool is_negative) { + if (align == Align::Default) + align = Align::Right; + Array buffer; const auto used_by_digits = convert_unsigned_to_string(value, buffer, base, upper_case); - auto used_by_prefix = sign_mode == SignMode::OnlyIfNeeded ? static_cast(is_negative) : 1; - if (prefix) { - if (base == 8) + size_t used_by_prefix = 0; + if (align == Align::Right && zero_pad) { + // We want String::formatted("{:#08x}", 32) to produce '0x00000020' instead of '0x000020'. This + // behaviour differs from both fmtlib and printf, but is more intuitive. + used_by_prefix = 0; + } else { + if (is_negative || sign_mode != SignMode::OnlyIfNeeded) used_by_prefix += 1; - else if (base == 16) - used_by_prefix += 2; - else if (base == 2) - used_by_prefix += 2; + + if (prefix) { + if (base == 8) + used_by_prefix += 1; + else if (base == 16) + used_by_prefix += 2; + else if (base == 2) + used_by_prefix += 2; + } } const auto used_by_field = used_by_prefix + used_by_digits; @@ -341,7 +353,7 @@ void FormatBuilder::put_u64( put_prefix(); put_digits(); put_padding(fill, used_by_right_padding); - } else if (align == Align::Right || align == Align::Default) { + } else if (align == Align::Right) { const auto used_by_left_padding = used_by_padding; if (zero_pad) { @@ -505,7 +517,7 @@ void Formatter::value>::Type>::format(TypeEra m_mode = Mode::Hexadecimal; m_alternative_form = true; - m_width = 2 * sizeof(void*) + 2; + m_width = 2 * sizeof(void*); m_zero_pad = true; } diff --git a/AK/Format.h b/AK/Format.h index c813d27e38..ff74e19799 100644 --- a/AK/Format.h +++ b/AK/Format.h @@ -204,8 +204,9 @@ private: Array m_data; }; -// We use the same format for most types for consistency. This is taken directly from std::format. -// Not all valid options do anything yet. +// We use the same format for most types for consistency. This is taken directly from +// std::format. One difference is that we are not counting the width or sign towards the +// total width when calculating zero padding for numbers. // https://en.cppreference.com/w/cpp/utility/format/formatter#Standard_format_specification struct StandardFormatter { enum class Mode { diff --git a/AK/Tests/TestFormat.cpp b/AK/Tests/TestFormat.cpp index fca71b7451..ea7fb5db19 100644 --- a/AK/Tests/TestFormat.cpp +++ b/AK/Tests/TestFormat.cpp @@ -111,6 +111,8 @@ TEST_CASE(zero_pad) EXPECT_EQ(String::formatted("{: <010}", 42), "42 "); EXPECT_EQ(String::formatted("{:010}", 42), "0000000042"); EXPECT_EQ(String::formatted("{:/^010}", 42), "////42////"); + EXPECT_EQ(String::formatted("{:04x}", -32), "-0020"); + EXPECT_EQ(String::formatted("{:#06x}", -64), "-0x000040"); } TEST_CASE(replacement_field) @@ -144,7 +146,7 @@ TEST_CASE(boolean_values) EXPECT_EQ(String::formatted("{:>4}", false), "false"); EXPECT_EQ(String::formatted("{:d}", false), "0"); EXPECT_EQ(String::formatted("{:d}", true), "1"); - EXPECT_EQ(String::formatted("{:#08x}", true), "0x000001"); + EXPECT_EQ(String::formatted("{:#08x}", true), "0x00000001"); } TEST_CASE(pointers) diff --git a/Applications/HexEditor/HexEditor.cpp b/Applications/HexEditor/HexEditor.cpp index 1b4d20d385..dfd99b9a1f 100644 --- a/Applications/HexEditor/HexEditor.cpp +++ b/Applications/HexEditor/HexEditor.cpp @@ -164,7 +164,7 @@ bool HexEditor::copy_selected_hex_to_clipboard_as_c_code() output_string_builder.appendff("unsigned char raw_data[{}] = {{\n", (m_selection_end - m_selection_start) + 1); output_string_builder.append(" "); for (int i = m_selection_start, j = 1; i <= m_selection_end; i++, j++) { - output_string_builder.appendff("{:#04X}", m_buffer.data()[i]); + output_string_builder.appendff("{:#02X}", m_buffer.data()[i]); if (i != m_selection_end) output_string_builder.append(", "); if ((j % 12) == 0) { @@ -516,7 +516,7 @@ void HexEditor::paint_event(GUI::PaintEvent& event) }; bool is_current_line = (m_position / bytes_per_row()) == i; - auto line = String::formatted("{:#010X}", i * bytes_per_row()); + auto line = String::formatted("{:#08X}", i * bytes_per_row()); painter.draw_text( side_offset_rect, line, diff --git a/Applications/HexEditor/HexEditorWidget.cpp b/Applications/HexEditor/HexEditorWidget.cpp index f63fc88117..09c140c0b7 100644 --- a/Applications/HexEditor/HexEditorWidget.cpp +++ b/Applications/HexEditor/HexEditorWidget.cpp @@ -54,7 +54,7 @@ HexEditorWidget::HexEditorWidget() m_editor = add(); m_editor->on_status_change = [this](int position, HexEditor::EditMode edit_mode, int selection_start, int selection_end) { - m_statusbar->set_text(0, String::formatted("Offset: {:#010X}", position)); + m_statusbar->set_text(0, String::formatted("Offset: {:#08X}", position)); m_statusbar->set_text(1, String::formatted("Edit Mode: {}", edit_mode == HexEditor::EditMode::Hex ? "Hex" : "Text")); m_statusbar->set_text(2, String::formatted("Selection Start: {}", selection_start)); m_statusbar->set_text(3, String::formatted("Selection End: {}", selection_end));