[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

This revision was automatically updated to reflect the committed changes. Closed by commit rGa37caebc2d2d: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper (authored by vsk). Changed prior to commit: https://reviews.llvm.org/D77843?vs=261558=261934#toc Repository: rG LLVM Github

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk marked an inline comment as done. vsk added inline comments. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:42-43 + llvm_unreachable("unsupported length"); +memcpy(reinterpret_cast(m_data), + reinterpret_cast(bytes), size); + }

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

labath accepted this revision. labath added a comment. Yeah, I think this looks good too. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:42-43 + llvm_unreachable("unsupported length"); +memcpy(reinterpret_cast(m_data), + reinterpret_cast(bytes),

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk marked 4 inline comments as done. vsk added inline comments. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:268 case GetPrintableElementType::UTF8: -return [](uint8_t *buffer, uint8_t *buffer_end, - uint8_t *) ->

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk updated this revision to Diff 261558. vsk marked an inline comment as done. vsk added a comment. Address review feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77843/new/ https://reviews.llvm.org/D77843 Files:

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM with minor comments, this is a big set of changes, I would prefer if one of the other reviewers also gave it a second look. Comment at:

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk updated this revision to Diff 261387. vsk added a comment. - Avoid heap memory management in "StringPrinterBufferPointer" entirely. - Rename "StringPrinterBufferPointer" to "DecodedCharBuffer", as that seems to more accurately reflect what the class is for. Repository: rG LLVM Github

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

shafik added inline comments. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:175 + constexpr unsigned max_buffer_size = 7; + uint8_t *data = new uint8_t[max_buffer_size]; + switch (escape_style) { I really wish we could get ride of the naked `new`.

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk added a comment. Friendly ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77843/new/ https://reviews.llvm.org/D77843 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk updated this revision to Diff 258825. vsk added a comment. Address review feedback: - Make 'escape_style' a regular parameter, use raw string literals Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77843/new/ https://reviews.llvm.org/D77843

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

labath added a comment. The test stuff looks good to me. Comment at: lldb/unittests/DataFormatter/StringPrinterTests.cpp:109 + EXPECT_TRUE(matches({"\0", 1}, QUOTE(""))); + EXPECT_TRUE(matches("\a", QUOTE("\\a"))); + EXPECT_TRUE(matches("\b", QUOTE("\\u{8}")));

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk updated this revision to Diff 258419. vsk marked an inline comment as done. vsk added a comment. Address review feedback: - Use standard gtest operators (EXPECT_EQ) - constexpr/std::move cleanup, comment cleanups, sprintf length fix Also, I took the opportunity to further unify the ASCII

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk marked an inline comment as done. vsk added inline comments. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:405 -template <> -bool StringPrinter::ReadStringAndDumpToStream< -StringPrinter::StringElementType::ASCII>( This is what I mean. The

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk marked 8 inline comments as done. vsk added inline comments. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:173 + unsigned escaped_len; + const unsigned max_buffer_size = 7; + uint8_t *data = new uint8_t[max_buffer_size]; shafik wrote: >

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

aprantl added inline comments. Comment at: lldb/unittests/DataFormatter/StringPrinterTests.cpp:1 +//===-- StringPrinterTests.cpp --*- C++ -*-===// +// drop the redundant C++ marker Repository: rG LLVM Github Monorepo CHANGES

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk planned changes to this revision. vsk added a comment. Sorry for the delay, I plan to have an update for this soon. In D77843#1978964 , @JDevlieghere wrote: > Have you asked the foundation people about the ASCII support in NSString? No, I'm not

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

labath added a comment. I haven't looked at the code in detail, but it seems ok to me. What does not seem ok is the hand-rolled matching in the tests. A more idiomatic approach would be to replace `isFormatCorrect` with a wrapper function which can be called from a test macro, and let gtest do

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

shafik added inline comments. Comment at: lldb/unittests/DataFormatter/StringPrinterTests.cpp:109 + EXPECT_TRUE(matches({"\0", 1}, QUOTE(""))); + EXPECT_TRUE(matches("\a", QUOTE("\\a"))); + EXPECT_TRUE(matches("\b", QUOTE("\\u{8}"))); We have [raw string

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

shafik added inline comments. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:50 + StringPrinterBufferPointer(StringPrinterBufferPointer &) + : m_data(rhs.m_data), m_size(rhs.m_size), m_deleter(rhs.m_deleter) { +rhs.m_data = nullptr;

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

JDevlieghere added a comment. This looks great, especially with the extra unit tests. Thanks Vedant. Have you asked the foundation people about the ASCII support in NSString? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77843/new/

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk marked an inline comment as done. vsk added inline comments. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:126 case 0: -retval = {"\\0", 2}; -break; +return {"\\0", 2}; case '\a': aprantl wrote: > We have a lot of implementations

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

aprantl added inline comments. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:126 case 0: -retval = {"\\0", 2}; -break; +return {"\\0", 2}; case '\a': We have a lot of implementations of this function all over our projects (`git grep

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk added reviewers: teemperor, labath. vsk added a comment. + Raphael, I suppose this is in a similar direction as D68691 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77843/new/ https://reviews.llvm.org/D77843

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

vsk created this revision. vsk added reviewers: JDevlieghere, davide. Herald added a subscriber: mgorny. Herald added a project: LLDB. Languages can have different ways of formatting special characters. E.g. when debugging C++ code a string might look like "\b", but when debugging Swift code the