This is an archive of the discontinued LLVM Phabricator instance.

[lldb][libc++] Adds chrono data formatters.
ClosedPublic

Authored by Mordante on Aug 29 2023, 11:15 AM.

Details

Summary

This adds the data formatters for chrono duration typedefs.

Diff Detail

Event Timeline

Mordante created this revision.Aug 29 2023, 11:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2023, 11:15 AM
Mordante requested review of this revision.Aug 29 2023, 11:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2023, 11:15 AM
aprantl added inline comments.Aug 29 2023, 12:45 PM
lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
996

Nice!

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
23

You probably copied & pasted this — this is no longer needed since every test function is now running in its own instance.

33

This is not guaranteed behavior and is highly specific on the actual compiler. I would probably just remove this first sets of tests.

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/main.cpp
17

can you change this to a function call like

std::cout()<<"break here\n";

It's not guaranteed that all compilers actually generate code for this line that results in a breakpoint opportunity.

Thanks for doing this!

Question to all: Should the summary string include the unit? lldb doesn't always show the type, so it could help comprehension if the unit is included. For example 60s instead of 60.

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
986–993

there's some regex grouping that doesn't seem to be needed.

Thanks for doing this!

Question to all: Should the summary string include the unit? lldb doesn't always show the type, so it could help comprehension if the unit is included. For example 60s instead of 60.

I was wondering about that too, but I wasn't sure whether that was wanted. If we do that what do you prefer for micro seconds us or µs. The latter uses Unicode. The C++ Standard allows both http://eel.is/c++draft/time.duration.io.

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/chrono/TestDataFormatterLibcxxChrono.py
23

I indeed copy-pasted the vector code; it's the first time I look at these formatters.

Mordante marked 4 inline comments as done.Sep 4 2023, 9:32 AM
Mordante added inline comments.
lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
986–993

Earlier testing without grouping didn't work. Since the next iteration uses units here is a regex per type.

Mordante updated this revision to Diff 555763.Sep 4 2023, 9:36 AM
Mordante marked an inline comment as done.

Address review comments.

Michael137 accepted this revision.Oct 21 2023, 5:28 PM

Was looking at leftover reviews, would be nice to land this

This revision is now accepted and ready to land.Oct 21 2023, 5:28 PM

Was looking at leftover reviews, would be nice to land this

Thanks for the review!

This revision was landed with ongoing or failed builds.Oct 25 2023, 10:37 AM
This revision was automatically updated to reflect the committed changes.

@Mordante @Michael137 This seems to fail on older versions of compiler/libcxx https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-matrix/7247/ You can probably fix this by just requiring a minimum clang version in the tests

@Mordante @Michael137 This seems to fail on older versions of compiler/libcxx https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-matrix/7247/ You can probably fix this by just requiring a minimum clang version in the tests

https://github.com/llvm/llvm-project/pull/70544