Re: [lmi] Not urgent: a plausible micro-optimization

[Vadim Zeitlin]

On Thu, 22 Feb 2018 23:26:47 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-02-22 13:31, Vadim Zeitlin wrote:
GC> [...]
GC> > assignment of wxFrame::GetChildren() to std::list<> does bother me because
GC> > this only compiles in STL build of wxWidgets and I'd like to be able to
GC> > build lmi with the default build not using STL.
GC> 
GC> Oh. I had assumed that by now a default wx build would use STL, and
GC> that the old containers were just kept for backward compatibility.

 Unfortunately it seems impossible to switch to using STL containers
without breaking compatibility in rather non-trivial ways. I did try to do
it more than once but failed...

GC> > 2. Replace the existing assignment with a copy. This is what I did in a
GC> >    new https://github.com/vadz/lmi/pull/69
GC> 
GC> I'm not opposed to accommodating the wx container classes. But I'd
GC> rather not use a for-loop to bring one element to the front [and I
GC> don't think that's exactly the loop you intended to write anyway].

 To be honest, for me the most straightforward way remains to write the
code in the way I had done originally, i.e. first update the active child
and then all the rest in a loop. All the other ones seem more or less
equally artificial to me.

GC> How about the following alternative?
GC> 
GC> 
---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
GC> diff --git a/skeleton.cpp b/skeleton.cpp
GC> index dc0d61015..11d3768cc 100644
GC> --- a/skeleton.cpp
GC> +++ b/skeleton.cpp
GC> @@ -1369,9 +1369,9 @@ void Skeleton::UpdateViews()
GC>  {
GC>      wxBusyCursor wait;
GC>  
GC> -    // Assignment implicitly ensures that this is not a wx legacy
GC> -    // container, and thus that std::list operations are valid.
GC> -    std::list<wxWindow*> z = frame_->GetChildren();
GC> +    // Make a local copy of the list for modification.
GC> +    wxWindowList const&  y = frame_->GetChildren();
GC> +    std::list<wxWindow*> z(y.begin(), y.end());
GC>      wxMDIChildFrame*     a = frame_->GetActiveChild();
GC>      // Bring any active child to front so it's updated first.
GC>      // It doesn't matter here if it's null: that's filtered below.
GC> 
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------

 This was actually my first attempt, and I should have mentioned it, but it
doesn't compile because wxWindowList::const_iterator doesn't satisfy the
input iterator requirements and in C++11 this ctor is only used when this
is the case.

 BTW, I had a lot of trouble seeing why exactly didn't it satisfy them with
g++ which simply listed all the overloaded ctor in its error message and
only gave this for the relevant one:

/usr/include/c++/6/bits/stl_list.h:705:9: note: candidate: template<class 
_InputIterator, class> std::__cxx11::list<_Tp, _Alloc>::list(_InputIterator, 
_InputIterator, const allocator_type&)
         list(_InputIterator __first, _InputIterator __last,
         ^~~~
/usr/include/c++/6/bits/stl_list.h:705:9: note:   template argument 
deduction/substitution failed:

while clang immediately gave the real reason in its error message:

/usr/bin/../lib/gcc/x86_64-linux-gnu/7.3.0/../../../../include/c++/7.3.0/bits/stl_list.h:709:2:
 note: candidate template ignored: substitution failure [with _InputIterator = 
wxWindowList::const_iterator]: no type named 'iterator_category'
      in 'std::iterator_traits<wxWindowList::const_iterator>'
        list(_InputIterator __first, _InputIterator __last,
        ^

which shows, once again, that using more than one compiler can be really
helpful and should explain why I'd like to keep lmi compilable with clang
when I start a discussion about this soon (spoiler: currently clang build
is broken).

 But to finish with this approach: to make it work, wx containers iterators
need to be changed and while it's certainly a good idea to do it, I don't
think you'd want to upgrade wx just for this, so it would be still nice to
have another solution for now.

GC> BTW, this:
GC>     z.remove(a);
GC>     z.push_front(a);
GC> seemed slightly ugly to me (remove() processes the whole list,
GC> and we only need the first occurrence, which should be unique;
GC> and some sort of swap function seems more fitting anyway),

 Yes, it doesn't seem natural to me. But, again, I'd rather avoid iterating
over the list more than once in the first place -- even if it's completely
negligible from the performance point of view, it's still unnecessary and I
don't think we gain anything in clarity by doing it neither. I.e. I still
believe that the best version was my original one...

 Regards,
VZ