This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Refactor C/C++ string and char summary providers
ClosedPublic

Authored by ljmf00 on Oct 27 2021, 1:24 PM.

Details

Summary
Signed-off-by: Luís Ferreira <contact@lsferreira.net>

This patch refactors C/C++ formatters to avoid repetitive code by using templates.

Diff Detail

Event Timeline

ljmf00 created this revision.Oct 27 2021, 1:24 PM
ljmf00 requested review of this revision.Oct 27 2021, 1:24 PM
ljmf00 edited the summary of this revision. (Show Details)Oct 27 2021, 1:24 PM
labath added a subscriber: labath.Oct 27 2021, 11:53 PM

Thanks for doing this. Just a couple of small remarks.

lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
55

We don't use anonymous namespaces like this. static is sufficient.

85–94

Maybe a helper function like pair<StringRef, Format> getElementTraits(StringElementType) would reduce the repetition further? Or possibly it could be a static array indexed by ElemType.

ljmf00 updated this revision to Diff 383121.Oct 28 2021, 12:36 PM

Thanks for doing this. Just a couple of small remarks.

Can you re-review?

lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
55

Done

85–94

Done. I used constexpr auto, I'm not sure if auto is a practice here in the LLVM codebase or I should explicitly specify the pair type.

labath accepted this revision.Oct 29 2021, 12:07 AM

Looks good.

Do you have commit access, or should I check this in for you (I can also fix up the formatting in that case)?

lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
37–40

This isn't the right formatting (brackets don't get their own line, and the first line is too long). Could you run the patch through clang-format please?

85–94

I'd probably spell out the type (and it looks like you already did).

This revision is now accepted and ready to land.Oct 29 2021, 12:07 AM
This revision was automatically updated to reflect the committed changes.

Do you have commit access, or should I check this in for you (I can also fix up the formatting in that case)?

Raphael tells me you don't, so I've committed this as rG5e31601.