After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 734230 - [review] refactor logging in NMDevice subclasses [th/bgo734230_device_logging]
[review] refactor logging in NMDevice subclasses [th/bgo734230_device_logging]
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-08-04 14:48 UTC by Thomas Haller
Modified: 2014-09-19 18:57 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Thomas Haller 2014-08-04 14:48:32 UTC
use the new logging macros _LOG[DIWE]() also for subclasses of NMDevice
Comment 1 Thomas Haller 2014-08-04 14:50:49 UTC
branch: th/bgo734230_device_logging
Comment 2 Dan Williams 2014-08-06 16:05:43 UTC
What not just:

+                (self) ? str_if_set (nm_device_get_iface ((NMDevice *) (self)), "(null)") : "(none)" \

instead of having self_to_device() which just does the same thing?  I would say it's a programmer error if that is not true everywhere we use these macros, plus you've added NM_IS_DEVICE() to nm_device_get_iface() so that will return NULL if for some reason we introduced a bug.
Comment 3 Thomas Haller 2014-08-06 16:25:41 UTC
(In reply to comment #2)
> What not just:
> 
> +                (self) ? str_if_set (nm_device_get_iface ((NMDevice *)
> (self)), "(null)") : "(none)" \
> 
> instead of having self_to_device() which just does the same thing?  I would say
> it's a programmer error if that is not true everywhere we use these macros,
> plus you've added NM_IS_DEVICE() to nm_device_get_iface() so that will return
> NULL if for some reason we introduced a bug.


At least while porting over to the new macros it was useful because I got a compile error if the "self" pointer was of an unexpected type.

Sometimes @self was the parent type NMDevice* or even "NMModem *self".

I could drop it, but I slightly prefer the additional compile time check.

??
Comment 4 Dan Williams 2014-08-06 16:33:52 UTC
I don't particularly like it, but I do see the utility of having it a compile-time check instead of a runtime check.  However, could we tighten that up with:

#define DECLARE_LOG_SELF(t) \
inline static NMDevice * \
_self_to_device (t *self) \
{ \
	return (NMDevice *) self; \
} \

and then in the files themselves after #include "nm-device-logging.h" we'd do:

DECLARE_LOG_SELF(NMDeviceTeam)

which would look cleaner to my eyes and be less code too.
Comment 5 Dan Williams 2014-08-06 19:09:22 UTC
Yeah, what's there now with _LOG_DECLARE_SELF() looks fine to me.