From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <4BCC71AF.4030903@domain.hid> Date: Mon, 19 Apr 2010 17:07:27 +0200 From: Gilles Chanteperdrix MIME-Version: 1.0 References: <4BCC619E.2@domain.hid> <4BCC6CEE.70907@domain.hid> <4BCC6E3C.3090301@domain.hid> <4BCC7092.1030809@domain.hid> In-Reply-To: <4BCC7092.1030809@domain.hid> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai-core] [RFC] fix XENO_OPT_DEBUG bugs. List-Id: Xenomai life and development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: xenomai-core Jan Kiszka wrote: > Gilles Chanteperdrix wrote: >> Jan Kiszka wrote: >>> Gilles Chanteperdrix wrote: >>>> Hi, >>>> >>>> I found some code which was referencing directly some >>>> CONFIG_XENO_OPT_DEBUG_ variables with things like: >>>> >>>> #ifdef CONFIG_XENO_OPT_DEBUG_FOO >>>> >>>> This usage is incompatible with the pre-requisites of the assert.h >>>> header that CONFIG_XENO_OPT_DEBUG_FOO should be defined at all times. >>>> While grepping for CONFIG_XENO_OPT_DEBUG_, I found that we also have >>>> many duplicates of construction like: >>>> #ifndef CONFIG_XENO_OPT_DEBUG_FOO >>>> #define CONFIG_XENO_OPT_DEBUG_FOO 0 >>>> #endif /* CONFIG_XENO_OPT_DEBUG_FOO */ >>>> >>>> So, a patch follows which: >>>> - replace the #ifdef with some #if XENO_DEBUG(FOO) >>> Should probably come as a separate patch. >> Come on, the patch is simple, one patch for this is enough. > > One part is an obvious fix, the other a refactoring under discussion. > >>>> - move all the initializations to assert.h >>>> >>>> This will make any reference to CONFIG_XENO_OPT_DEBUG_FOO outside of >>>> assert.h suspicious, and easy to detect. >>> How many duplicates did you find? >> A lot, especially for CONFIG_XENO_OPT_DEBUG_NUCLEUS > > That's because the CONFIG_XENO_OPT_DEBUG_NUCLEUS defines were misplaced. > Besides that we had one CONFIG_XENO_OPT_DEBUG_QUEUE duplicate, but tons > of proper users. I don't see such an immediate need. > > When adding this kind of switching, it's till more handy to have things > locally in your subsystem. That also makes reviewing easier when you > only find changes in files that belong to a subsystem. That is not the main point, the main point is that putting all these defines in one files allow to detect easily direct references to the symbols. > >>> Generally, I'm more a fan of decentralized management here (e.g. this >>> avoids needless patch conflicts in central files). >> If we maintain the list in alphabetical order (which I have done), we >> reduce the likeliness for conflicts. The aim of doing this is also that >> I can check that the sources are clean with: >> >> find xenomai-2.5 ! -name 'assert.h' -name '*.[ch]' | xargs grep CONFIG_XENO_OPT_DEBUG_ >> >> And that I can add this test to the automated build test. >> >> Note that forgetting to add a #define to the list yields an immediate >> compilation error. So, the patch makes things completely safe. > > What did you change to make this happen (for new users of XENO_DEBUG)? Nothing. If you forget to add the define, and do not enable the debug option (which everybody does most of the time), you get a compilation error. What I changed, however, is that the above find | grep allows immediately to detect direct users of the symbols. -- Gilles.