Bug 26642 - Add proper debug flag support
Summary: Add proper debug flag support
Status: RESOLVED MOVED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: mission-control (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/ei...
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2010-02-18 16:13 UTC by Eitan Isaacson
Modified: 2019-12-03 19:34 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Eitan Isaacson 2010-02-18 16:13:36 UTC
Currently MC5 is stuck in a limbo between integer verbosity debug levels, and debug flags. Proper debug flag support should be added, while keeping the debug level for backwards compatibility.

I have a branch set up with this. The debug flag names are pretty arbitrary, they could be refined as we continue, and as they are used.
Comment 1 Simon McVittie 2010-02-19 02:17:31 UTC
Ah, sorry, I said this on the wrong bug:

I notice your branch removes an extern function. Please don't do that (it would break osso-mission-control on Maemo unless this function is unused, and I happen to know that mcd_debug_set_level() is called by osso-mission-control).
Comment 2 Eitan Isaacson 2010-02-19 16:05:59 UTC
Sorry, didn't realize there were external consumers.

I returned a cleaner version of mcd_debug_set_level() to my branch.
Comment 3 Simon McVittie 2010-02-22 03:29:13 UTC
Bah. I'm sure I reviewed this already, but the combination of Bugzilla and unstable 3G on a train ate it.

Basically, the problem is that osso-mission-control uses mcd-debug.h, and because a lot of that header is inline, it hard-codes the version that was current at the time it was compiled. Also, I don't want to expand MC's API/ABI - I'm trying to stop src/ having any public API at all (in favour of just having OMC use libmission-control-plugins) but we're not there yet.

So, I think we should introduce a debug-internal.h, which defines McdDebugFlags, _mcd_debug_flag_is_set (prefix an underscore to make it internal to MC), and your version of DEBUG() (only if DEBUG_FLAG is defined). All MC files should include debug-internal.h instead of mcd-debug.h.

mcd-debug.h should keep defining DEBUG() too (to support OMC), but only if DEBUG_FLAG is *not* defined.

mcd-debug.h should continue to define _mcd_debug_get_level for source compatibility.

> -gint mcd_debug_level = 0;

This must continue to exist; otherwise, OMC will abort on startup with a linker error. (Don't remove extern symbols.)

-------------------

Review comments that don't relate to the general approach:

> +                       mcp_set_debug ((flags != 0));

This should check MCD_DEBUG_PLUGIN - mcp is the plugins library

> + #define DEBUGGING mcd_debug_flag_is_set (DEBUG_FLAG)

Parenthesize the definition, since it's an expression: #define DEBUGGING (_mcd_debug_flag_is_set (DEBUG_FLAG))

>  {
> -    mcd_debug_level = level;
> +               if (level == 1)
> +               {
> +                flags = ~MCD_DEBUG_TREES;

What's going on with the indentation? 4-space indentation for existing files, Telepathy style (GNUish 2+2 space indentation) for entirely new files, and no tabs anywhere, please.
Comment 4 Simon McVittie 2010-02-22 03:30:22 UTC
Please reinstate the patch keyword when this is ready for review again.
Comment 5 Eitan Isaacson 2010-02-22 23:00:02 UTC
Did the changes you suggested.
Hope I found all the whitespace issues.
Comment 6 GitLab Migration User 2019-12-03 19:34:09 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-mission-control/issues/20.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.