749645 - Use fallback files in hal/ when a backend isn't implemented instead of having randomly defined methods

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I tried to apply an old patch a volunteer wrote for a MacOS Battery API backend and the experience was terrible: given that most backends are currently defining methods for un-implemented stuff in a sort of generic cpp file (WindowsHal.cpp, LinuxHal.cpp, etc.) it is very annoying to add something especially given that MacOS is using FallbackHal.cpp which is basically including most of the methods.

Anyway, having multiple FallbackFoo.cpp files makes it easier to just implement a chunk in a backend.

Comment 1

[Justin Lebar (not reading bugmail)]

I'm ready to r+ this, but what do you think about this solution:

Everybody builds FallbackHal.cpp, which has code like

  #ifndef MOZ_HAS_HAL_POWER
  void FrobPower() { }
  void Reboot() { }
  #endif

  #ifndef MOZ_HAS_HAL_SCREEN
  void FrobScreen() { }
  #endif

This way, you never have to remember to add FallbackFoo.cpp to all the relevant platforms.  It'll just work, and if you mess up the #defines, it will fail to link on *every* platform, not just some of them.

Comment 2

[Mounir Lamouri (:mounir)]

But that requires to #define MOZ_HAS_HAL_SCREEN somewhere to platforms implementing it so this is only moving the problem somewhere else, right?

Comment 3

[Justin Lebar (not reading bugmail)]

Oh, I you're right -- we'd have to define it in a header file somewhere.  :-/  I had this delusion that we could define it in the CPP file.

Comment 4

[Justin Lebar (not reading bugmail)]

What about if we had a single HalScreen.cpp file:

HalScreen.cpp:

  #if defined(XP_WIN)
  #include <hal/WindowsScreen.cpp>
  #else
  void FrobScreen();
  #endif

Again, this simplifies the make logic and, more importantly, ensures that every platform always gets an implementation of the full Hal interface.

Comment 5

[Justin Lebar (not reading bugmail)]

Er, that should be:

  #if defined(XP_WIN)
  #include <hal/WindowsScreen.cpp>
  #else
  void FrobScreen() {}
  #endif

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I have a very strong preference for the solution that Mounir has implemented here.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 619057 [details] [diff] [review]
Patch v1

Get a try run in, of course.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cda84ca70452