1221477 - Define macro to declare BluetoothService functions

Status: RESOLVED INVALID

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Replace function declaration with macro in
- BluetoothServiceBluedroid
- BluetoothDBusService
- BluetoothServiceChildProcess

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Define macro to declare BluetoothService functions

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8683003 [details] [diff] [review]
Patch 1 (v1): Define macro to declare BluetoothService functions

Review of attachment 8683003 [details] [diff] [review]:
-----------------------------------------------------------------

Hi,

I honestly don't know if I should r+ this patch. I know that we have other places with similar macros, but I really dislike this style of programming.

 * It obfuscates the source code.
 * It's harder to 'git grep' for the affected interfaces.
 * Using macros here is C-style inheritance, and the ISO C++ committee deprecates the use of macros and C style where possible.

Maybe other team members can share their opinion or give some rational for this patch.

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I'm not a big fan of using macros to wrap up virtual functions. But I'm curious is there any guideline in gecko.

Hi Kyle,
Do you have any suggestion? Since we saw this kind of styles in gecko in many places.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We do this for XPCOM interfaces (e.g. NS_DECL_ISUPPORTS, NS_DECL_NISFOO) so I don't see anything wrong with it.

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi David,
Do we encourage to use these macros in gecko? I would say Comment 1 still makes sense to me. However, doing this for XPCOM interfaces seems to be a classic example, do you have any opinion?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think there's a particular guideline one way or the other.

In the case of the XPCOM uses, the macros are autogenerated from the IDL, so they generally make maintenance easier, since unimplemented or extra methods are caught by the compiler.

Whoever maintains/owns the code in question should probably decide what they consider more maintainable.

Thorough usage of the override keyword is also a good idea here (and I'd note the patch does improve this, although it could be improved without macros as well).

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> Whoever maintains/owns the code in question should probably decide what they
> consider more maintainable.

Maybe we should simply vote on this and go with what ever the majority decides. The wiki lists Ben, Shawn, Jocelyn, Bruce, Will, Jamin and me as team members. Is there anyone else who should have a vote? Is Jamin still active?

Comment 8

[Bruce Sun [:brsun]]

For me, I would treat this kind of changes (i.e. using macro to declare or to define tones of same functions in several places) as the last strategy to improvement the readability and the maintainability if we don't have other choices.

There could be lots of styles to use macros to declare functions, but the following two styles might be very common in gecko:
 - (1) Use one macro to declare and define additional functions (ex. NS_INLINE_DECL_THREADSAFE_REFCOUNTING[1]).
 - (2) Use one macro to declare function prototypes (ex. NS_DECL_NSIRUNNABLE[2]), and let programmers to define functions by themselves (ex. NS_IMETHODIMP nsRunnable::Run(){...}[3]).

No matter whether it is used for reference counting or for runnables, they are used to achieve some basic and fundamental functionality. They might worth to use macro to hide the implementation details or to formalize function prototypes because they could be used in lots of places very easily. They are relatively small and represent themselves as basic building blocks.

Comparing between these two styles, codes which are covered by style (1) are easier be used to me. When using macros in style (2), programmers have to know more about the function prototypes and make sure all corresponding functions declared by the macro are implemented properly by themselves. But since runnables only have one Run() function needs to be implemented, the overhead to write codes to use macros in style (2) probably is acceptable to most maintainers.

In this patch, |BT_DECL_SERVICE| hides 70~80 function declarations in one macro; which is good because this macro saves tones of codes for programmers. But it also introduces overhead by doing so: programmers needs to dig into the details of |BT_DECL_SERVICE| to understand how those 70~80 functions are declared, which might not be very intuitive. Although |BluetoothService| is the basic building block of the Bluetooth module, |BT_DECL_SERVICE| is used in three classes at most currently. The benefit we could have by introducing |BT_DECL_SERVICE| might be different from what we already have by using conventional macros (i.e. reference counting or runnables).

I am more interested in what will be changed next after applying this patch. If there are some further plans to make our codes easier to be maintained, and this patch is just one part of it, then we might be able to compare the trade-off with a more global perspective. Otherwise, we might need to figure out what we could get to overwhelm the overhead due to the indirectness of this new macro.

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#538
[2] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/include/nsIRunnable.h#38
[3] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/include/nsIRunnable.h#87

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> In this patch, |BT_DECL_SERVICE| hides 70~80 function declarations in one
> macro; which is good because this macro saves tones of codes for
> programmers.

An important point is the read/write ratio of the code. The ref-counting macros are often written when implementing classes and are *the* public interface to the ref-counting functionality. Developers don't have to know the macro's interface or implementation to use them. The ratio leans towards writing, so these macros certainly make sense.

In our case, |BT_DECL_SERVICE| and similar macros are written once or twice in sub-classes, not often changed, and afterwards mostly read by developers. Implementing them still requires developers to know which interfaces the macros contain. So a class' implementations contain methods that are not (visibly) part of the class declaration.

> I am more interested in what will be changed next after applying this patch.
> If there are some further plans to make our codes easier to be maintained,
> and this patch is just one part of it, then we might be able to compare the
> trade-off with a more global perspective. Otherwise, we might need to figure
> out what we could get to overwhelm the overhead due to the indirectness of
> this new macro.

At some point, I want to land bug 1088574, which implements the current backend API with BlueZ. Most of the managers and services could probably be shared between the existing backends. The current sub-classes could then be merged into the base class. I guess this would remove one fourth to one third of the Bluetooth code. The need for the related macros would go away.

Many of my recent cleanups and updates in the backend code are done with this goal in mind. But I don't have a time frame for landing such a change. It's definitely a long term effort.

Comment 10

[Ben Tian (inactive)]

(In reply to Bruce Sun [:brsun] from comment #8)
> I am more interested in what will be changed next after applying this patch.

My reason for macro is to simply remove duplicate code change each time we add new methods in BluetoothService. For example, patch 3 in bug 1181479 requires 3x identical code change for newly added methods.

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8683003 [details] [diff] [review]
Patch 1 (v1): Define macro to declare BluetoothService functions

I prefer to do refactoring to reduce these methods instead of introducing macros.

Comment 12

[Will Wang [:WillWang]]

ni myself for posting opinion later.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I'd prefer to not use macros for the reasons I gave in the discussion.

But I don't have a strong opinion on it either. My main concern is that we agree on the style and direction of the Bluetooth module.

Comment 14

[Bruce Sun [:brsun]]

Comment on attachment 8683003 [details] [diff] [review]
Patch 1 (v1): Define macro to declare BluetoothService functions

If we are going to adapt the macro solution to save the duplication, I would prefer to split those functions into small groups (ex. by profiles) into different macros, and let managers reuse those macros to normalize the interfaces. It would only scratch where it itches to save duplications in managers by adapting reusable codes further. The idea and the purpose is good, but f- for the huge macro v.s. re-usability.

Recap my previous comment 8, using macros to improvement the readability would be the last strategy to me if we really don't have any other choices. But as stated on comment 9, there would be other solutions to address the same issue. Probably it is something worth to try before we decide to adapt the macro strategy. Just my two cents.

Comment 15

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8683003 [details] [diff] [review]
Patch 1 (v1): Define macro to declare BluetoothService functions

Review of attachment 8683003 [details] [diff] [review]:
-----------------------------------------------------------------

Personally, I prefer not to hide many function declarations in one macro.

I believe there are many people who have different opinions on this topic.
Sometimes, style is a matter of personal taste.

For me, I try to avoid using macro in c++ unless it's necessary since macro operates at textual level rather than syntax level and sometimes it's hard to understand. It might also mess up compiler error message.

I’d like to echo #comment 14, I think multiple grouped macro would be better than one huge macro.
In addition, it's a little pity to me that BT_DECL_SERVICE would overwrite the last modified history of each line which “git blame” showed.

Code simplicity counts and the macro does help.
However, I prefer to make code more explicit rather than terse.

Comment 16

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8683003 [details] [diff] [review]
Patch 1 (v1): Define macro to declare BluetoothService functions

Review of attachment 8683003 [details] [diff] [review]:
-----------------------------------------------------------------

This patch do eliminate lots of codes here, which is good, but I personally tend to *not* to hide all of our function declarations in a huge macro due to readability and maintainability.
And personally I only want to use macro if necessary or for simple things that developers won't need to dig in to the details of that macro.
Since we probably want to see these function definitions frequently during development, I prefer not to use macro for it.

Comment 17

[Will Wang [:WillWang]]

Easing the pain of the "3x identical code" is exciting to me, I'm a fan of eliminating this kind of smell, clean code makes me happy; and I noticed that |BT_DECL_SERVICE| also add override specifier to some functions which better have it, this improve the code quality.

On the other hand,
indirection introduced by macro also bring some cons which I will concern about:
- obfuscates the source code, as Thomas mentioned
- tools or many code tracing techniques like static analysis, UML,... etc,even grep, will be weaken, and I like these tools.

For me, I would consider landing this patch or not is both OK, since the patch was written and bring some pros.
And I would take this situation as technical debt and we better refactor it once we are available.

To dealing with this kind of refactoring, we might treat it as the "long class" smell in the refactoring book[1], and possible refactorings for long class are:
1. extract class [2]
2. extract subclass [3]
3. extract interface [4]
4. extract superclass [5]

(There are some related refactorings which may be helpful, however I will not list for now.)

[1] Refactoring: Improving The Design of Existing Code
[2] http://refactoring.com/catalog/extractClass.html
[3] http://refactoring.com/catalog/extractSubclass.html
[4] http://refactoring.com/catalog/extractInterface.html
[5] http://refactoring.com/catalog/extractSuperclass.html

Comment 18

[Will Wang [:WillWang]]

- refactoring catalog for your reference: http://refactoring.com/catalog/
- fix nit born from rephrasing: s/dealing/deal

Comment 19

[Ben Tian (inactive)]

5 "no" votes, 1 "yes", and 1 neutral for the patch. Resolve this bug as invalid.