I can confirm that Boost's visit_each() is called when
   : //public boost::enable_shared_from_this<Klass>,
is uncommented.

When I change the call to visit_each() in visit_each_type() from
  visit_each(limited_action, _A_functor);
to either
  sigc::visit_each(limited_action, _A_functor);
or
  ::sigc::visit_each(limited_action, _A_functor);
then one of libsigc++'s visit_each() specializations is called, but the called
specialization is not the correct one. The slot is not disconnected, not even
if
   : //public boost::enable_shared_from_this<Klass>,
is commented out.

Created attachment 269605 [details] [review]
visit_each becomes sigc::visit_each; uses class templates to provide better compiler hints

When discussing the problem with various calls of visit_each resolving to incorrect specializations/overloads, James Lin pointed out the following article: http://www.gotw.ca/publications/mill17.htm

I ran with a suggestion in that article to correct this:
> Moral #2: If you're writing a function base template, prefer to write it as a single function template that should never be specialized or overloaded, and then implement the function template entirely as a simple handoff to a class template containing a static function with the same signature. Everyone can specialize that -- both fully and partially, and without affecting the results of overload resolution.

Ok, so we've got lots of overloaded visit_each() template functions but no
template specializations of visit_each() in ligsigc++. The article that you
linked to in comment 2 clearly explains the difference.

I've tried to understand why the compiler sometimes chooses the wrong overload
and I think I understand at least a few situations.

1. With the unscoped call 
     visit_each(limited_action, _A_functor);
   in visit_each.h, visit_each_type(), the compiler can make argument-
   dependent lookup. (See e.g. Bjarne Stroustrup, "The C++ Programming
   Language" 4th edition, 26.3.6.) visit_each() functions in any namespace
   can be candidate functions in overload resolution.

2. With a scoped call
     sigc::visit_each(limited_action, _A_functor);
   argument-dependent lookup is turned off. Only visit_each() functions in
   namespace sigc can be candidate functions, and (important) only functions
   declared before the call are candidate functions. Depending on the order
   of inclusion of the header files, more or less visit_each() functions are
   candidate functions. In the case I tested, only the basic visit_each()
   functions defined in visit_each.h is a candidate.

3. The test case libsigc++2/tests/test_functor_trait shows strange results,
   like you mentioned in
   https://mail.gnome.org/archives/libsigc-list/2014-February/msg00000.html
   That's because of these calls to visit_each() in compose.h:

    //Note that the AIX compiler needs the actual template types of
    //visit_each to be specified:
    typedef typename type_functor::setter_type type_functor1;
    visit_each<T_action, type_functor1>(_A_action, _A_target.functor_);
    typedef typename type_functor::getter_type type_functor_getter;
    visit_each<T_action, type_functor_getter>(_A_action, _A_target.get_);

   When two template arguments are explicitly specified in the calls, only
   visit_each() functions with exactly two template parameters are candidate
   functions. (Probably also functions with more than two parameters, where
   the extra parameters have default values.) Most visit_each() functions in
   libsigc++ have more than two template parameters. That's why only a few
   visit_each() functions are candidates for the calls from visit_each() in
   compose.h.

   test_functor_trait gives a much more reasonable result if the explicitly
   specified template arguments are removed. (It's the same result as
   indicated in the modified test_functor_trait.cc in the patch in comment 2.)

   I don't understand why the AIX compiler needs extra hints in the calls
   to visit_each() in compose.h, but in no other calls to visit_each().

I have not yet tested the patch. I'll do that. I assume that it works, but I
also assume that it will break API. visit_each() is not just an internal
utility in libsigc++. Application programs can add their own adaptors and
corresponding visit_each() functions.
See https://developer.gnome.org/libsigc++/unstable/structsigc_1_1adapts.html

I can spot three issues here.

1. Calls of visit_each<x, y>() in compose.h

The explicit template arguments were added 2005-02-01,
https://git.gnome.org/browse/libsigc++2/commit/?id=1891ba3d954012c5d55425256d38ac4322171124
On the same day visit_each.h was changed in the same way,
https://git.gnome.org/browse/libsigc++2/commit/?id=5fff8d4d4ef775aad72a9e0ee5ccc1b15b0b0ab5

On the next day the newly added explicit template arguments were removed from
visit_each.h, but not from compose.h.m4,
https://git.gnome.org/browse/libsigc++2/commit/?id=f1f5e3c094bfd4fb2495cce77e876274f826b069

On 2005-04-26 explicit template arguments were again added in visit_each.h,
https://git.gnome.org/browse/libsigc++2/commit/?id=6b150419bb219d0ea7b0e3cb8bc27b39b08b945a
and once again removed on the next day,
https://git.gnome.org/browse/libsigc++2/commit/?id=50b2f065db0567d37a65d4b46bbd5248e907b93d

Explicit template arguments can't be used in calls to visit_each().
They restrict the set of functions that take part in overload resolution.
They must be removed from compose.h.m4. They should have been removed 9 years
ago, when they were removed from visit_each.h.

2. Calls to sigc::visit_each()

In mem_fun.h, adaptor_trait.h and track_obj.h, visit_each() is called with an
explicit namespace scope, sigc::visit_each().
In mem_fun.h and adaptor_trait.h sigc:: was added as a fix to bug 165222,
https://git.gnome.org/browse/libsigc++2/commit/?id=23fd1e06e78f5f60f23997a5b8fdc73466d2097f
https://git.gnome.org/browse/libsigc++2/commit/?id=8974eae3915750e819382db3bdc4f4c0e425e9c8

track_obj.h is a fairly new file that I've added, probably without knowing
exactly what I was doing. I think I just copied from mem_fun.h, when I decided
to call sigc::visit_each() instead of visit_each().
https://git.gnome.org/browse/libsigc++2/commit/?id=b0727f4e8cbb289d66b11fc29adf133f6c86847e

As I noted in comment 3, item 2, an explicit namespace scope also restricts
the set of functions that take part in overload resolution. That was the
intension when bug 165222 was fixed. I suspect though that in most cases
such restrictions are not wanted. Those explicit namespace scopes can probably
cause other problems that no one has reported yet. It took 9 years before
someone reported the similar problem with sigc::compose().

3. boost::visit_each() is called

The patch in comment 2 solves this problem, but at a high price. If a user of
libsigc++ wants to add her/his own adaptor, she/he must add a template
specialization of struct visitor in namespace sigc::internal.

There are two hard-to-combine requirements on libsigc++.
  a. Restrict the search for the best visit_each() in some way, e.g.
     restrict it to namespace sigc, or at least exclude namespace boost.
  b. Do not restrict the search for the best visit_each() to namespace sigc.
     Bug 486373 shows that at least one user of libsigc++ implements his own
     adaptor.

I don't like a situation where some users of libsigc++ must add some of their
own code to namespace sigc.

(In reply to comment #4)
> The patch in comment 2 solves this problem, but at a high price. If a user of
> libsigc++ wants to add her/his own adaptor, she/he must add a template
> specialization of struct visitor in namespace sigc::internal.
> 
> There are two hard-to-combine requirements on libsigc++.
>   a. Restrict the search for the best visit_each() in some way, e.g.
>      restrict it to namespace sigc, or at least exclude namespace boost.
>   b. Do not restrict the search for the best visit_each() to namespace sigc.
>      Bug 486373 shows that at least one user of libsigc++ implements his own
>      adaptor.
> 
> I don't like a situation where some users of libsigc++ must add some of their
> own code to namespace sigc.

I don't know if there's a really good way around that, and after consulting with some of my peers, I'm not sure I agree that it is -too- much of a problem.  There are precedents for specializing template classes based in another namespace (even including std):

  - http://stackoverflow.com/questions/8513417/what-can-and-cant-i-specialize-in-the-std-namespace
  - Interesting tangent: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3730.html

I understand that it's unpalatable to specialize a type in the sigc namespace, but in light of the precedents above, can you think of any ways in which this might backfire?  Does it unduly restrict developers?  Is it otherwise a technically correct solution?

If deemed to be technically correct, I'd suggest ditching the 'internal' nested namespace, and sticking with something like what I have below.  (Optionally, I guess one could also consider placing `struct visitor` inside something like a sigc::ext(ernal), sigc::public, or sigc::visitor.  Either way, that's really something for you, Murray, and others to decide on.  ;))

If you'd like to proceed with something like this, I can produce a cleaner patch with `struct visitor` placed in whichever namespace it fits best.

> namespace sigc {
>    template <typename T_functor>                        // <--- anyone may specialize this
>    struct visitor
>    {
>       template <typename T_action>
>       static void
>       visit(const T_action& _A_action, const T_functor& _A_functor)
>       {
>          _A_action(_A_functor);
>       }
>    };
> 
>    template <typename T_action, typename T_functor>
>    void
>    visit_each(const T_action& _A_action, const T_functor& _A_functor)
>    {
>       sigc::visitor<T_functor>::visit(_A_action, _A_functor);
>    }
> }

And then, yes, if a developer wants a customized visitor, she'd need something like

> namespace MyVendor {
>    struct MyThing { ... };
> }
> 
> namespace sigc {
>    template <>
>    struct visitor<MyVendor::MyThing>
>    {
>       template <typename T_action>
>       static void
>       visit(const T_action& _A_action,
>             const MyVendor::MyThing& _A_functor)
>       {
>          _A_action(_A_functor.some_custom_thing());
>       }
>    };
> }

I think you're right. It's acceptable that some users of libsigc++ must put
a small part of their code in namespace sigc, provided it's not
sigc::internal. We've already got the macros SIGC_FUNCTORS_HAVE_RESULT_TYPE,
SIGC_FUNCTOR_TRAIT and SIGC_FUNCTORS_DEDUCE_RESULT_TYPE_WITH_DECLTYPE that
must be used in namespace sigc.

I still have two concerns with your proposal.

1. It breaks API. Only users that add their own overloads of visit_each()
will be affected. I wonder if such an API break can be accepted. I think
that's up to Murray to decide. At least one user has added his own
visit_each() overload, or else bug 486373 would not have been filed.

(Not a concern) The proposed change does not break ABI. I've looked in the
object library files (.so files) of libsigc++, glibmm and gtkmm. There is no
visit_each there.

2. I don't understand why your proposal works.

When visit_each() in visit_each.h calls
sigc::internal::visitor<T_functor>::do_visit_each(_A_action, _A_functor)
the compiler obviously finds the correct specialization of struct visitor,
even though (as is always the case) the best specialization is defined after
the call to do_visit_each(). Is that required by the C++ standard, or is it
an extension in gcc 4.8.1, which I've used?

I've seen statements to the effect that when a class or struct template must
be instantiated, only specializations defined before the point of
instantiation are considered. In this case the struct is not instantiated, but
its static member function is. Are template specializations searched for in
the whole namespace in such a case? In this case no argument-dependent lookup
is involved, is it?

(In reply to comment #6)
> I still have two concerns with your proposal.
> 
> 1. It breaks API. Only users that add their own overloads of visit_each()
> will be affected. I wonder if such an API break can be accepted. I think
> that's up to Murray to decide. At least one user has added his own
> visit_each() overload, or else bug 486373 would not have been filed.

Yeah, I agree this is a problem.  I just haven't thought of any decent way
to get around it.  Personally, I don't often attempt "clever" solutions,
as they have a tendency to explode in my face.  :)

Barring anyone coming up with a non-API-breaking solution, would it be
conceivable to combine this patch with a version bump to 2.3.0?  (How does
libsigc++2's versioning work?)

> 2. I don't understand why your proposal works.
> 
> When visit_each() in visit_each.h calls
> sigc::internal::visitor<T_functor>::do_visit_each(_A_action, _A_functor)
> the compiler obviously finds the correct specialization of struct visitor,
> even though (as is always the case) the best specialization is defined after
> the call to do_visit_each(). Is that required by the C++ standard, or is it
> an extension in gcc 4.8.1, which I've used?

Unfortunately I can't answer that, either.  I went ahead and (naively)
downloaded a draft of the standard, but I couldn't quickly discern the
lookup rules.  I admit that I had just cargo-culted a solution from Herb
Sutter's site, believing it to be true because a) it's Herb Sutter, and b)
the results were what I had hoped for.

I might try some more Googling to see if anyone has proposed an easy-to-digest
version.

> I've seen statements to the effect that when a class or struct template must
> be instantiated, only specializations defined before the point of
> instantiation are considered. In this case the struct is not instantiated, but
> its static member function is. Are template specializations searched for in
> the whole namespace in such a case? In this case no argument-dependent lookup
> is involved, is it?

You'll see in my patch that -all- calls to visit_each are now qualified by
the sigc namespace, so ADL doesn't apply.

I think we'll need to find a C++ language lawyer...  :P

I've also tried to understand the name lookup rules in a draft C++ standard.
It's definitely _not_ easy to understand. After having read small parts of the
standard, I'm not even sure if ADL shall consider whole namespaces, or only
what's declared or defined before the point of template function
instantiation. (Wherever that exactly is.)

I've come to like your proposed solution in comment 2, except that struct
visitor shall be located in namespace sigc instead of sigc::internal. That's
easy to change. But it's risky to implement it, if we're not sure it's
standard C++. We may have problems with other compilers.

The present way of finding the right visit_each() overload has worked well
most of the time for many years. The only problem is that the wrong overload
may be chosen, if there are unrelated functions named visit_each().

An alternative solution would be to rename visit_each() to something that is
less likely to clash with names in unrelated code, for instance call it
sigc_visit_each(). Unfortunately, this solution breaks API just as much as
your solution with struct visitor.

> Barring anyone coming up with a non-API-breaking solution, would it be
> conceivable to combine this patch with a version bump to 2.3.0?  (How does
> libsigc++2's versioning work?)

The latest version is 2.3.1.
The versioning rules for libsigc++ and many other packages are
- If the second part of the version number (here 3) is odd, it's an unstable
  version. If it's even, it's a stable version.
- In an unstable version API can be added, changed or removed.
- A stable version, x.(y+2).0, may contain new API, and new deprecations, not
  in the previous stable version, x.y.z.
  An upgrade from x.y.z to x.(y+2).0 shall not require change or recompilation
  of application programs.
- Within a stable version, x.y.*, API or ABI is not changed.
- The next major release, (x+1).y1.z1, is incompatible with x.y.z.

If we stick to the rules, an API break requires a version bump to 3.y.z.
I think that's several years ahead. I wonder if we could allow breaking the
rules in a case such as this one, where probably very few users of libsigc++
would be affected. I don't want to decide. It would not be the first such rule
break. The glib developers are trying to implement API-breaking modifications
that are arguably rule-breaking. (bug 687659, bug 698614)

Murray, what do you think about my discussion in comment 8, especially the
last paragraph? Would it be acceptable to introduce an API-breaking change
that affects only users of libsigc++ that have added their own overloads of
sigc::visit_each()?

I haven't looked at this properly yet, but a small API change for an unusual usage is OK, particularly if it's not an ABI break.

This looks like it definitely needs a test case added to the build at about the same time as any fix.

Created attachment 272286 [details]
test2.cc

Bjarne Stroustrup discusses template instantiation in "The C++ Programming
Language", 4th edition, chapter 26, especially section 26.3.3 "Point-of-
Instantiation Binding".

I think I have misunderstood where the point of instantiation (PoI) is, and
therefore I have misunderstood the implications of the rule that only template
class specializations defined before the PoI are considered.
Referring to the attached test case: The PoI of struct visitor<> is not, as I
thought, in sigc::visit_each(), at the call to visitor<>::do_visit_each().
The PoI is just before the start of main().

Bjarne's examples are not as indirect as the calls to visitor<>::
do_visit_each() in the attachment and in a modified libsigc++. I'm still
almost certain that the PoI of struct visitor<> is where an instantiation is
needed, which is typically much later in the code than the call to
visitor<>::do_visit_each() from visit_each(). 

If I'm right, all standard-compliant C++ compilers should find the correct
specialization of sigc::visitor<>.

I suggest that we replace the visit_each() overloads by specializations of
strict visitor<>, like Ryan has suggested. Some work remains:

A test case would certainly be good. Hopefully someone can then test the
modified libsigc++ with other compilers before it's implemented in a stable
release. I'll try to create a good test case. It shall not use Boost or any
other external library, but it shall select the wrong visit_each() when ADL
is used. It shall contain a user-defined adaptor, not in namespace sigc.

The patch in comment 2 must be modified before it's suitable for
implementation.

The test case in comment 0 shows that a visit_each() in Boost can be selected
when one in libsigc++ should have been selected. Is the opposite scenario
possible? Can sigc::visit_each() be selected when boost::visit_each() should
have been selected? If Boost also uses ADL to find a visit_each(), I suppose
that could happen. Even though that's really Boost's problem, rather than
libsigc++'s, if such a wrong selection is possible, we should perhaps also
change the name of visit_each() and perhaps also of visit_each_type().
It looks a bit funny with sigc::sigc_visit_each(), but if it solves a real
problem, it would be acceptable.

Created attachment 272492 [details] [review]
patch: Replace visit_each() overloads by struct visitor<>

Modified version of Ryan's patch in comment 2.

Created attachment 272493 [details] [review]
patch: Tests: Add test_visit_each

A test case.

It would be great, if someone could test with other compilers than gcc.

Can I push these patches? The patch in comment 12 breaks API, but not ABI.
Only users that have added their own visit_each() overloads are affected by
the API break. Unfortunately their programs will still compile, but there will
be run-time errors, if they rely on auto-disconnection of slots.

(In reply to comment #11)
> Bjarne Stroustrup discusses template instantiation in "The C++ Programming
> Language", 4th edition, chapter 26, especially section 26.3.3 "Point-of-
> Instantiation Binding".

I'm glad you dug out the Stroustrup text.  I added a TODO for the same, but I was pulled to a different project and hadn't had time.  :/

(In reply to comment #13)
> It would be great, if someone could test with other compilers than gcc.

I can probably test things out easily enough with llvm/clang on Linux and OS X, and then on Windows with Microsoft Visual C++ 2008 and 2010.  (I believe I have entitlements for 2012/2013, but those aren't installed in my environment atm.)  How soon would you need to have results?

> Can I push these patches? The patch in comment 12 breaks API, but not ABI.
> Only users that have added their own visit_each() overloads are affected by
> the API break. Unfortunately their programs will still compile, but there will
> be run-time errors, if they rely on auto-disconnection of slots.

Given the time it took to track down the original bug, I'm hoping that pushing such a patch somehow draws enough attention directly to it s/t responsible parties learn about the change before their applications begin crashing.  :)

Ex:  It'd be nice if the ChangeLog had a nice big warning to distro maintainers letting them know of the API break.  (They'll need to audit dependent packages, checking for custom instances of visit_each.  If any such instances exist, they ought to mark the combination of old package+new libsigc++ broken.)

(In reply to comment #14)
> How soon would you need to have results?

There's no definit deadline. This is probably more urgent for you than it
is for me.

> It'd be nice if the ChangeLog had a nice big warning to distro maintainers
> letting them know of the API break.

We don't update the ChangeLog in the git repository any more. When a new
release is made, a ChangeLog file for the tarball is generated from the commit
messages. I should have included a warning in the commit message in the patch.
I'll do that before I push anything to the git repository.
There should also be a warning in the NEWS file.
I can post a message on libsigc-list@gnome.org, but I don't how many people
subscribe to that list.


I also hope for a confirmation from Murray, that it's alright to push this
API-breaking patch, if it passes the tests with some other compilers.
The last paragraph in comment 13 summarizes the implications.

Ryan, any progress? Have you tested my patch with other compilers?

Murray, would it be acceptable to push this API-breaking patch.
See the last paragraph in comment 13.
What shall we do to inform affected users?

I have tested with clang++ 3.4.

First I noticed that some of the old test cases did not compile, because
unused functions were defined. I've pushed a patch that fixes that.
https://git.gnome.org/browse/libsigc++2/commit/?id=1ce41a0a59214d896d2dead06f5431440a8b6c34

After that fix, both 'make' and 'make check' succeeded without the patches in
comment 12 and comment 13. But clang++ does not accept the "double template"
in the specializations of struct visitor, e.g.

  template <>
  template <class T_type, bool I_derives_trackable>
  struct visitor<limit_reference<T_type, I_derives_trackable> >

../sigc++/limit_reference.h:114:1: error: extraneous template parameter list
      in template specialization [-Werror]
template <>
^~~~~~~~~~~

When I remove 'template <>', both clang++ and g++ accept the code, and all
test cases pass, including the new test_visit_each.

I don't know what's most standards-compliant, with or without 'template <>'.

Ryan, have you abandoned this bug, or do you intend to test with other
compilers? You've mentioned MSVC++.

(In reply to comment #17)
> Ryan, have you abandoned this bug, or do you intend to test with other
> compilers? You've mentioned MSVC++.

I'm unfortunately guilty of deprioritizing non-critical work after running behind on a few projects.  What I had hoped was a trivial "drop in new source and rebuilt" task turned out to be more complicated.  Our build machines compile libsigc++ using (seemingly) custom authored Microsoft Visual Studio project files which exclude the test directories.

I'll try to budget more time for this within the next two weeks so that I can be less of a deadbeat w/r/t contributing something useful back to this project.

Created attachment 280714 [details] [review]
patch: Replace visit_each() overloads by struct visitor<>

A modified patch. This one is accepted by both gcc and clang.
I removed trailing blanks in the modified files. That's why this patch is
longer than the one it replaces.

Created attachment 280715 [details] [review]
patch: Tests: Add test_visit_each

A modified test case. This one is accepted by both gcc and clang.

Created attachment 281169 [details] [review]
patch: Add test_visit_each to MSVC_Net2010

I have updated MSVC_Net2010 in the git repository to include project files for
all test cases now in the repository.
https://git.gnome.org/browse/libsigc++2/commit/?id=0896e284b75397e6b7cb9312fdd39268c4172df0

I have not found an easy way to run the tests with Visual C++. I don't know if
there is one. I ran them manually one by one in a console window. All test
cases pass, except test_bind_refptr, which crashes. I don't know why, but it's
not related to the introduction of struct visitor<>. It crashes both with and
without that modification.

I have tested the patches in comment 19 and comment 20 with Visual C++ 2010
Express on Windows 8.1. No problems. test_visit_each passes.

Like clang++, VC++ does not accept "double templates" such as
  template <>
  template <class T_type, bool I_derives_trackable>
  struct visitor<limit_reference<T_type, I_derives_trackable> >

I have now tested
  on Linux Ubuntu 14.04 with gcc 4.8.2
  on Linux Ubuntu 14.04 with clang 3.4
  on Windows 8.1 with Visual C++ 2010

Tests with still other compilers and/or operating systems are of course
welcome, but not important any more. I feel reasonably confident that this
will work with any full-blown C++ compiler.


Concerning the "double template" declarations, I found this example in
a draft C++ standard:

  template <class T> struct eval; // primary template

  template <template <class, class...> class TT, class T1, class... Rest>
  struct eval<TT<T1, Rest...>> { }; // partial specialization

A declaration of a specialization can have more template parameters than the
declaration of the primary template.

"template <> template <>" is used for a template within a template, such as

  template<class T> struct string {
    template<class T2> int compare(const T2&);
  };
  template<class T> template<class T2> int string<T>::compare(const T2& s) {}

(In reply to comment #21)
> All test
> cases pass, except test_bind_refptr, which crashes. I don't know why,

Now I know why. See bug 564005 comment 24.

Ryan, do you still intend to test sigc::visitor<> with other compilers?
It's not important now that I have tested with three compilers.

(In reply to comment #22)
> Ryan, do you still intend to test sigc::visitor<> with other compilers?
> It's not important now that I have tested with three compilers.

I applied the patches, rebuilt with Visual Studio 2013, and **all tests passed**.

Side notes:

1.  test_cpp11_lambda and test_track_obj could also define USING_CPP11_LAMBDA_EXPRESSIONS if _MSC_VER >= 1800 (Visual Studio 2013).  I'll search for / file a bug about this.  (1700 / Visual Studio 2012 would also probably work.)

2.  Contrary to comments in test_lambda.cc, MSVC 2013 does *not* like the conversion from std::ostream to bool.  (I'll also file a separate bug about this.)  #if 0'ng out this section allowed me to compile and run the test program.

27>E:\src\git\libsigc++2\sigc++/functors/slot.h(103): error C2440: 'return' : cannot convert from 'std::ostream' to 'bool'

I have pushed the patches in comments 19-21.

Users of libsigc++ are informed of the API break in
https://mail.gnome.org/archives/libsigc-list/2014-July/msg00003.html
and in bug 486373 comment 7.

This bug is fixed. The issues with Visual Studio 2013 are not related to
visit_each()/struct visitor. They can be discussed and fixed in other bugs.

(In reply to comment #23)
> Side notes:

1. Bug 733752.

2. What happened? I haven't seen a bug about this one.

2. Bug 734368