Avoid type collision by renaming callback variable

[artokin]

Description

Jenkins jobs are failing due build error:

Compile: arm_hal_timer.cpp
[Error] arm_hal_timer.cpp@28,9: reference to 'callback' is ambiguous
[Error] arm_hal_timer.cpp@50,5: reference to 'callback' is ambiguous
[Warning] arm_hal_timer.cpp@19,15: 'callback' defined but not used [-Wunused-variable]
[ERROR] ./mbed-os/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_timer.cpp: In function 'void timer_thread(const void*)':
./mbed-os/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_timer.cpp:28:9: error: reference to 'callback' is ambiguous
         callback();
         ^
./mbed-os/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_timer.cpp:19:15: note: candidates are: void (* callback)()
 static void (*callback)(void);
               ^
In file included from ./mbed-os/rtos/rtos/Thread.h:27:0,
                 from ./mbed-os/rtos/rtos/rtos.h:25,
                 from ./mbed-os/hal/api/mbed.h:22,
                 from ./mbed-os/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_timer.cpp:9:
./mbed-os/hal/api/Callback.h:3524:33: note:                 template<class T, class R, class A0, class A1, class A2, class A3, class A4> mbed::Callback<R(A0, A1, A2, A3, A4)> mbed::callback(const volatile T*, R (T::*)(A0, A1, A2, A3, A4) const volatile)
 Callback<R(A0, A1, A2, A3, A4)> callback(const volatile T *obj, R (T::*func)(A0, A1, A2, A3, A4) const volatile) {

Fix the error by renaming callback as arm_hal_callback.

[artokin]

Originally this change was made to ARMmbed/nanostack-hal-mbed-cmsis-rtos#12 but that repository is going to be retired.

@kjbracey-arm would you please check again.

[kjbracey]

I'm fine with it, but can't help but feel that mbed.h adding "callback" into the global namespace is somewhat asking for trouble with a wide range of possible applications and libraries.

(Could there be some way to stop mbed.h doing "using namespace mbed")?

[0xc0170]

@geky @sg- I think this alternative is the easiest to fix the failure (compare to renaming callback in the Callback API, or changing mbed.h file). What do you think?

(Could there be some way to stop mbed.h doing "using namespace mbed")?

I personally also would like to remove using from any header file. That's something for a discussion that should happen soon, most likely be fixed for Q4 release.

[geky]

related issue #2653

[pan-]

Another possible fix would be to take advantages of the C++ namespace rules.

There is two way to do it:

• When callback is used, indicate explicitly that it is the one from the global namespace by prefixing it with ::

static void timer_thread(const void *)
{
    for (;;) {
        osSignalWait(1, osWaitForever);
        // !!! We don't do our own enter/exit critical - we rely on callback
        // doing it (ns_timer_interrupt_handler does)
        //platform_enter_critical();

        // USE CALLBACK FROM THE GLOBAL NAMESPACE
        ::callback();
        //platform_exit_critical();
    }
}

// Not called while running, fortunately
void platform_timer_set_cb(void (*new_fp)(void))
{
    // USE CALLBACK FROM THE GLOBAL NAMESPACE
    ::callback= new_fp;
}

• Import only the symbols name needed by this file:

//#include "mbed.h"
// DO NOT import mbed.h, pick only the symbols needed
// Include timeout and timer instead
#include "Timeout.h"
#include "Timer.h"

 #include "platform/arm_hal_timer.h"
 #include "platform/arm_hal_interrupt.h"

// bring useful symbols in the global namespace
using mbed::Timeout;
using mbed::Timer;

[sg-]

I think this can be merged given it was previously but we also have a lot of work to do in general to rid this. More to come on this soon.

[sg-]

/morph test

[mbed-bot]

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 855

All builds and test passed!

[kjbracey]

@pan- Does ::callback work? I thought the using directive had added callback to the global namespace, so that would remain ambiguous.

A name prefixed by the unary scope operator :: (5.1) is looked up in global scope,
in the translation unit where it is used. The name shall be declared in global
namespace scope or shall be a name whose declaration is
visible in global scope because of a using-directive (3.4.3.2).

[0xc0170]

As @pan- highlighted this brings a question if mbed-os files should include mbed header file. I would say no, go with his second proposal, to include only what is required. Thoughts?

[pan-]

@kjbracey-arm

The standard can be confusing on this point.
Basically a using directive add all namespace enclosed name into the current scope for unqualified lookup while a using declaration add the name specified into the current scope for qualified and unqualified lookup.

The section 7.4 explain this:

A using-directive specifies that the names in the nominated namespace can be used in the scope in which the using-
directive appears after the using-directive. During unqualified name lookup (3.4.1), the names appear as if they were
declared in the nearest enclosing namespace which contains both the using-directive and the nominated namespace.
[Note: in this context, “contains” means “contains directly or indirectly”. — end note ]

More importantly:

3 A using-directive does not add any members to the declarative region in which it appears.

And finally there is an example of this in the standard:

namespace D {
  int d1 ;
  void f( char );
}

using namespace D;
int d1 ; // OK: no conflict with D::d1

namespace E {
  int e ;
  void f( int );
}
namespace D { // namespace extension
  int d2 ;
  using namespace E;
  void f( int );
}

void f ()
{
  d1++; // error: ambiguous ::d1 or D::d1?
  ::d1++; // OK
  D::d1++; //OK
  d2++; // OK: D::d2
  e++; // OK: E::e
  f(1); // error: ambiguous: D::f(int) or E::f(int)?
  f('a’); // OK: D::f(char)
}

[kjbracey]

Right, I didn't know that distinction.

So after using mbed::foo;, ::foo would actually be mbed::foo, so a global definition of foo would be an error.

But after using namespace mbed;, we can also define our own global foo, and disambiguate references to it via ::foo.

[pan-]

@0xc0170 Explicitly stating which name from another namespace is used is the best way to minimize name collision. In python we all know that we shouldn't write from X import * but it is what mbed.h does with the standard library and mbed namespace. The worse part is that every file including mbed.h became polluted by these using directives.

There is an old but still interesting article from Herb Sutter explaining how to properly use namespaces: http://www.gotw.ca/publications/migrating_to_namespaces.htm .

@kjbracey-arm Correct, that's the big difference between using directive and using declaration. A using declaration add the name in the declarative region of the current scope; using directive doesn't.

[0xc0170]

As @pan- highlighted this brings a question if mbed-os files should include mbed header file. I would say no, go with his second proposal, to include only what is required. Thoughts?

Any update for this PR, ppl happy with it as it is or we follow one of the suggestions @pan- did above?

[kjbracey]

I'm okay with this, as a local change, it's just as reasonable as the suggestions above.

But #2663 seems like it would solve the problem more generally. If that was in, this wouldn't need to be.

[sg-]

Will land this and something that looks like #2663 will also land for mbed OS.