[PATCH] D149675: [Demangle] convert itaniumDemangle and nonMicrosoftDemangle to use std::string_view

Thu Jun 1 10:43:46 PDT 2023

nickdesaulniers added a comment.

In D149675#4313980 <https://reviews.llvm.org/D149675#4313980>, @MaskRay wrote:

> In D149675#4313962 <https://reviews.llvm.org/D149675#4313962>, @efriedma wrote:
>
>> LGTM
>>
>> In D149675#4313944 <https://reviews.llvm.org/D149675#4313944>, @nickdesaulniers wrote:
>>
>>> In D149675#4313933 <https://reviews.llvm.org/D149675#4313933>, @efriedma wrote:
>>>
>>>> Maybe it makes sense to revert D149104 <https://reviews.llvm.org/D149104> while you're fixing the various demanglers to correctly respect the input length?
>>>
>>> This should fix-forwards the test. Sorry it took me so long to root cause.
>>
>> If I understand the situation correctly, all the other demanglers read past the end of the buffer too; we just get lucky that none of the tests on the buildbots use the API in a way which exposes that.  I'd prefer to revert to known working, then fix each demangler, then fix the generic demangle() API.
>
> Sounds good. So we need two commits
>
> - Revert D149104 <https://reviews.llvm.org/D149104>. The description of this patch can be used by the revert commit.

Done https://reviews.llvm.org/rG3e3c6f24ff85ea52ed67d4c26f1d3d0eacd1ad1b.

> - Combine D149104 <https://reviews.llvm.org/D149104> and this patch

I don't plan to combine them and would prefer to keep them distinct patches to minimize the amount of code that may get reverted (if at all). D149104 <https://reviews.llvm.org/D149104> was a "top down" approach to the interface. The necessity of the revert shown it should be done bottom up instead.

This patch, as well as D149784 <https://reviews.llvm.org/D149784> and D151003 <https://reviews.llvm.org/D151003> are bottom up.  I also need to write a patch for the M$ demangler.  Then I will revisit the capstone D149104 <https://reviews.llvm.org/D149104>.

> Both commits need very sophisticated testing (including `check-lldb check-bolt`)...

Done (for this patch).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149675/new/

https://reviews.llvm.org/D149675