FEATURE_COMMON_PAL doesn't compile in master

[tommikas]

Since #2496 was merged in, it looks like.

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:2636: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) {
                                 ^

[geky]

Hi @tommikas, unfortunately the callback name conflicts with the recently added callback function. This is an unfortunate side effect of the implicit mbed namespace expansion in the mbed.h header. If you remove the mbed.h header and include the needed files directly (for example "Timer.h" and "Timeout.h") the relevant classes (including the callback function) will be in the appropriate namespace.

[jupe]

do we get fix for this? We doesn't want to create workarounds for this..

[geky]

The general expansion of the mbed namespace could be changed to a specific set of classes (aka all of the classes currently exposed), omitting the callback function. I can look into making a pr, although I don't know if it's too late to remove the callback function from the general namespace.

@pan-, @0xc0170, thoughts? Explicit using statements would give us better control of what is exposed.

It would be better to eliminate the mbed namespace expansion, but I don't know a path forward without breaking everything.

[bridadan]

Potentially crazy idea, but what if we moved all of the important includes into a file called mbed_includes.h (or maybe mbed_internal.h), then just include this new file in mbed.h and add the different using keywords in mbed.h? That way we maintain compatibility with all the user programs out there (which do want the implicit mbed namespace) but then just use mbed_includes.h within the tree for when these name spacing issues get in the way?

[teetak01]

Please revert the related PR. Mbed Client applications are broken now.

[sg-]

If you remove the mbed.h header and include the needed files directly (for example "Timer.h" and "Timeout.h") the relevant classes (including the callback function) will be in the appropriate namespace.

this is not a solution. We propose people use mbed.h as an entry point. Seems a better name would have been mbed_callback?

Reverting.

edit: err 💥 cant revert since too much else relies on this. Just need to fix it and ASAP!

[geky]

#2661 provides a solution based on my previous proposal: (#2653 (comment))

Moves callback to mbed::callback even if the mbed.h file is included. An alternative would be to rename callback -> mbed_callback but that would take a bit more time.

@bridadan, We'd really want to avoid this form of name polution even in the user-space. Maybe ultimately adding an mbed-os.h that drops the namespace expansion is the best way to go?

[sg-]

It would be better to eliminate the mbed namespace expansion

Don't think this is an option, that ship has sailed as they say way long ago.

I don't know a path forward without breaking everything.

Forget that namespace exists and I think the answer will become more clear.

[geky]

It would be better to eliminate the mbed namespace expansion

Don't think this is an option, that ship has sailed as they say way long ago.

Fair enough

I don't know a path forward without breaking everything.

Forget that namespace exists and I think the answer will become more clear.

If I forgot the namespace exists, I would have suggested adding it ; )

[geky]

Alternative patch: #2663

[pan-]

Potentially crazy idea, but what if we moved all of the important includes into a file called mbed_includes.h (or maybe mbed_internal.h), then just include this new file in mbed.h and add the different using keywords in mbed.h? That way we maintain compatibility with all the user programs out there (which do want the implicit mbed namespace) but then just use mbed_includes.h within the tree for when these name spacing issues get in the way?

@bridadan Users can already pick up the files they need. What are the benefits of including everything ?

@geky mbed.h pull every symbol from the mbed and std namespace into the global namespace there is no fix or workaround for that beside not using mbed.h.

[bridadan]

@pan- The benefit is that's how mbed applications have worked in the past, so this wouldn't be a breaking change. But if user's needed to have more control over namespaces then they could always include the mbed_internal.h instead of mbed.h. mbed.h has always been the easy entry point. mbed_internal.h (or whatever the appropriate name is) could provide more flexibility for other users where mbed.h is too polluting.

[ciarmcom]

ARM Internal Ref: IOTMORF-460

[kjbracey]

To be C++ standard-ish, #include <mbed> might be logical, as .h standard library headers go into global namespace, and unsuffixed ones go into only std.

Alternatively, put the using directives into an #ifndef, so the user can do #define MBED_NOT_GLOBAL_NAMESPACE(?) or something at the top of a file.

[geky]

Those are good thoughts. Does any other library do this: #include <mbed>? I suspect that option would require additional work for exporters.

[sg-]

@tommikas is this resolved?

[tommikas]

Yes. Closing.