labath marked 5 inline comments as done.
labath added inline comments.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/main.cpp:5
+{
+ std::forward_list<int> empty{}, one_elt{47}, five_elts{1,22,333,4444,55555};
+ return 0; // break here
----------------
jingham wrote:
> 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???
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).
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:141
-namespace lldb_private {
-namespace formatters {
-class LibcxxStdListSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
+class ForwardListFrontEnd: public AbstractListFrontEnd {
+public:
----------------
jingham wrote:
> 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?
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.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:153
+ std::map<size_t, ListIterator> m_iterators;
+};
+
----------------
jingham wrote:
> Why are these three ivars not in the AbstractListFrontEnd? They appear in
> both derived classes and seem to be used in the same way.
Good question. I started with two completely distinct implementations and then
worked towards merging them. I guess did not finish the job.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:182
+ m_head = nullptr;
+ return false;
}
----------------
jingham wrote:
> 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?
It doesn't matter since they are re-initialized in HasLoop(), but it does look
weird. Fixed it.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:251-260
+ ListIterator current(m_head);
+ if (idx > 0) {
+ auto cached_iterator = m_iterators.find(idx - 1);
+ if (cached_iterator != m_iterators.end()) {
+ current = cached_iterator->second;
+ actual_advance = 1;
+ }
----------------
jingham wrote:
> This use of the iterators also seems like it should be shared.
Indeed it can.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:299-301
+ /*m_node_address = backend_addr->GetValueAsUnsigned(0);
+ if (!m_node_address || m_node_address == LLDB_INVALID_ADDRESS)
+ return false;*/
----------------
jingham wrote:
> ?
Oops.
https://reviews.llvm.org/D35556
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
