Page MenuHomePhabricator

Add data formatter for libc++'s forward_list
ClosedPublic

Authored by labath on Jul 18 2017, 5:51 AM.

Details

Summary

This adds a data formatter for the implementation of forward_list in
libc++. I've refactored the existing std::list data formatter a bit to
enable more sharing of code (mainly the loop detection stuff).

Event Timeline

labath created this revision.Jul 18 2017, 5:51 AM

Hey Jim,

I wrote a couple of new libc++ data formatters in july. You seem to be the data formatter owner nowadays. Would you mind taking a look?

jingham edited edge metadata.Oct 23 2017, 2:19 PM

Will do. I was sick all last week so I have a bit of a backlog, I'll get to this as soon as I can.

jingham requested changes to this revision.Oct 25 2017, 1:42 PM

It looks like the behavior of the two derived list classes here are only partially factored out. See the individual comments but it looks like there is a lot more that could be shared.

I also don't think we should change the naming conventions used for these classes piecemeal. If we want to simplify the names and hide the FrontEnds in the source files we should do that wholesale, and not one by one. Otherwise next time I read this file I'm going to waste time wondering why the ForwardListFrontEnd isn't specific to LibCxx whereas the LibcxxStdListSyntheticFrontEnd does seem to be...

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/main.cpp
6

Some compilers (including clang at other times) will omit debug info for variables that it doesn't see getting used. I usually try to use the variables I'm going to print (call size on them, pass an element to printf, whatever...) to keep this from happening.

OTOH, it's nice if compilers don't do that, so maybe relying on their not doing that in our tests is a useful forcing function???

source/Plugins/Language/CPlusPlus/LibCxxList.cpp
117

I wonder about the decision to move these classes out of lldb_private::formatters. They don't need to be in a globally visible namespace, but all the others are. They also all have a consistent naming convention, which this one doesn't have. This doesn't seem like the sort of thing we should change piecemeal, that will just lead to confusion.

Was there some other reason for doing this that I'm missing?

129

Why are these three ivars not in the AbstractListFrontEnd? They appear in both derived classes and seem to be used in the same way.

178

The AbstractFrontEndList has the m_fast_runner and m_slow_runner but they are only reset in the Update for the LibcxxStdListSyntheticFrontEnd, and not here. Is that on purpose?

267–276

This use of the iterators also seems like it should be shared.

315–317

?

319–326

This bit is done in exactly the same way in the two sub-classes. Could it be moved into the base class Update?

This revision now requires changes to proceed.Oct 25 2017, 1:42 PM
labath marked 5 inline comments as done.Oct 26 2017, 2:32 PM
labath added inline comments.
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/main.cpp
6

Hmm... I'd say we can leave it like this. I'd be surprised if the compiler can figure out that these declarations are a no-op without very aggressive inlining (which it just won't do at -O0).

source/Plugins/Language/CPlusPlus/LibCxxList.cpp
117

There's no hard reason, but I was trying to make these names more concise

I've put them in the anonymous namespace, as llvm coding style encourages that for file-local entities https://llvm.org/docs/CodingStandards.html#anonymous-namespaces. I'd be happy to move the other formatters into the anonymous namespace as well, as I have a couple of other formatters in the pipeline, which already are in the anon namespace.

As for the names, if I followed the existing style, this would read something like

class LibCxxStdForwardListSyntheticFrontEnd: public LibCxxAbstractListSyntheticFrontEnd

which would be formatted horribly, as it does not fit a single line. As these are file-local entities, I think we can be a bit less verbose.

I had not renamed the std::list class because of the way I wrote this change (make a copy of the std::list formatter, and then factor out the common code), but you are right that they should follow the same convention. I propose to shorten the name of the other class as well.

129

Good question. I started with two completely distinct implementations and then worked towards merging them. I guess did not finish the job.

178

It doesn't matter since they are re-initialized in HasLoop(), but it does look weird. Fixed it.

267–276

Indeed it can.

315–317

Oops.

labath updated this revision to Diff 120491.Oct 26 2017, 2:33 PM
labath edited edge metadata.
labath marked 4 inline comments as done.

Address review comments.

jingham accepted this revision.Oct 30 2017, 10:51 AM

Great, this is much nicer.

source/Plugins/Language/CPlusPlus/LibCxxList.cpp
117

The effect on choosing names is one of the many reasons why I don't favor the llvm 80 character restriction. But we're stuck with it so...

This revision is now accepted and ready to land.Oct 30 2017, 10:51 AM

Thanks.

By the way, I have a couple of other patches lined up that you probably did not notice:
https://reviews.llvm.org/D35666
https://reviews.llvm.org/D35615
https://reviews.llvm.org/D35305

This revision was automatically updated to reflect the committed changes.