ColinGilbert / mili

Automatically exported from code.google.com/p/mili
Boost Software License 1.0
0 stars 0 forks source link

Overloading an operator with templates in global namespace is just evil. You shall not do that. #40

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
Suppose a template function in global  namespace:
template <class Enum>
inline bitwise_enum<Enum> operator | (Enum a, Enum b)
{
    return bitwise_enum<Enum>(a) | bitwise_enum<Enum>(b);
}

This will overload and instantiate any operators with arbitrary type of 
operands:

#include "bitwise_enums.h"

enum E {A = 1, B = 2};

int main (int argc, const char * argv[])
{
    int x = A | B;
}

What is the expected output? What do you see instead?
It should compile.
The above would not compile because it selects the above template, and there is 
no conversion from bitwise_enum to int. Anyway, it should not select this 
operator at all.

Please provide any additional information below.
There is no easy solution to this problem, unless using strong type safe enums 
proposed in C++0x.
A more elaborated solution would involve to use traits for selected types of 
enums and use these traits to enable the operators defined in global namespace. 
Using boost type traits and boost utility library may help to implement this.

Btw, embedding the bitwise_enum into a namespace does NOT solve the problem.

Original issue reported on code.google.com by agro...@onlinehome.de on 31 Aug 2011 at 2:06

GoogleCodeExporter commented 9 years ago
Hi, thanks for reporting the issue.

Why embedding the bitwise_enum into a namespace does not solve the problem?
For example, the following works:
-------------

struct S{};

namespace N{ // commenting to get the error

template <class Enum>
inline S operator | (Enum a, Enum b)
{
    return S();
}

}

enum E{ uno, dos };
int f(E x, E y)
{
   return x | y;
}
-----------------------

Original comment by danielgutson@gmail.com on 1 Sep 2011 at 3:24

GoogleCodeExporter commented 9 years ago
I will explain this below, but the fact that you overload operators for ALL 
possible operands in the global namespace is really a bad decision.
Consider this, assuming the operators are in global namespace:

int x = 1;
int y = 5 | x; // This will select the overloaded operators!

OK, now why putting it in a namespace would not help much:
The problem here is, that you need to define all enums which will be used for 
bitwise_enum need to be placed in the same namespace where the overloaded 
operators are defined. This restricts your choices to place enums into 
arbitrary namespaces to avoid name clashes considerable.

After your example, I put some code which illustrates this problem.

OK, function f would work here as usual using the built-in operators. But this 
is not the problem, the problem is to use bitwise_enum:

In order to make enums use the overloaded operators, you would need to place 
them im namespace N:

Suppose I have a free function in namespace my:

namespace my {
  inline int foo(int flags) { return flags; }
}

Now I would like to use bitwise_enums for better type safety for parameter 
"flags". Ideally, I would do this:

namespace my {
    namespace foo_flags {
       enum E {A=1, B=2, C=4};
    }   
    namespace other_flags {
       enum E {A=1, B=2, C=4};
    };

    typedef bitwise_enum<foo_flags::E> FooFlags;

    inline FooFlags foo(FooFlags flags) { return flags; }
}

and:
  my::FooFlags x = my::foo(FooFlags::A | FooFlags::B);

This will not work as such, however. There are ways to accomplish this, but 
this requires a more elaborated implementation. Well, and since C++0x we have 
strong typed enums anyway. (Nonetheless, if you are interested I can send you 
more information and code which will work in C++03.

In order to use my::foo using your bitwise_enum, I would have to do the 
following:

In order the compiler selects the overloaded operators, all enums for 
bitwise_enum must be placed in their dedicated namespace N, or wherever 
bitwise_enum and the operator overloads are defined.
In order to avoid name clashes we have to prefix them, or make them unique in 
another way:
namespace N {
   enum FooFlags_E { FooFlags_A = 1, FooFlags_B = 2, FooFlags=4};
   enum OtherFlags_E { OtherFlags_A = 1, OtherFlags_B = 2, OtherFlags=4};    
}

namespace my {
      inline N::bitwise_enum<N::FooFlags_E> foo(N::bitwise_enum<N::FooFlags_E> flags) { return flags; }
}

and:

N::bitwise_enum<N::FooFlags_E> flags = my::foo(N::FooFlags_A | N::FooFlags_B);

compare this with:
my::FooFlags flags = my::foo(FooFlags::A | FooFlags::B);

Original comment by agro...@onlinehome.de on 1 Sep 2011 at 9:47

GoogleCodeExporter commented 9 years ago
Have you seen the MILI_NAMESPACE macro?

Whatever, this has been reported too many times.
We'll fix this by explicitly telling what enums will be treated with 
bitwise_enum, in a code similar to this one:

template <class Enum>
struct EnumEnabler
{
    enum { EnabledConversion = false };
};

template <class Enum, bool EnabledConversion>
struct EnumMapper
{
    typedef int ReturnType;
    static int operation(Enum e1, Enum e2)
    { return e1|e2; }
};

struct S{};

template<class Enum>
struct EnumMapper<Enum, true>
{
    typedef S ReturnType;
    static S operation(Enum e1, Enum e2)
    { return S(); }
};

template <class Enum>
typename EnumMapper<Enum, EnumEnabler<Enum>::EnabledConversion>::ReturnType 
operator|(Enum e1, Enum e2)
{
    return EnumMapper<Enum, EnumEnabler<Enum>::EnabledConversion>::operation(e1, e2);
}

enum Enum1{ E11, E12 };
enum Enum2{ E21, E22 };

int f(Enum1 e1, Enum1 e2)
{
    return e1|e2;
}

template <>
struct EnumEnabler<Enum2>
{
    enum { EnabledConversion = true };
};

S f(Enum2 e1, Enum2 e2)
{
    return e1|e2;
}

So, everything the user will have to do is
    template <>
    struct EnumEnabler<Enum2>
    {
        enum { EnabledConversion = true };
    };
which can be put into a macro for better convenience.

Please confirm that this will fix your issue; meanwhile, we'll start 
implementing it and will be present in the coming new version of the library 
(GenericExceptions is delaying the release).

Original comment by danielgutson@gmail.com on 2 Sep 2011 at 9:14

GoogleCodeExporter commented 9 years ago
(of course, S represents bitwise_enum, and "operation" would be one per bitwise 
operation)

Original comment by danielgutson@gmail.com on 2 Sep 2011 at 9:20

GoogleCodeExporter commented 9 years ago

Original comment by danielgutson@gmail.com on 5 Oct 2011 at 3:35

GoogleCodeExporter commented 9 years ago
Hi,
can you please check the branch "branches/branch-issue-40" and confirm if what 
we changed fixed your issue?

Original comment by adrianre...@gmail.com on 20 Nov 2011 at 6:44

GoogleCodeExporter commented 9 years ago

Original comment by daniel.g...@fudepan.org.ar on 9 Jan 2012 at 3:18