Re: [MERGE] debugs message improvement: use HERE macro wherever sensible

On 12.11.2012 08:25, Alex Rousskov wrote:
> On 11/11/2012 06:46 AM, Kinkie wrote:
>> On Sun, Nov 11, 2012 at 2:13 AM, Alex Rousskov wrote:
>>> On 11/10/2012 07:57 AM, Kinkie wrote:
>>>
>>>> +//TODO: just before branching 3.3, blanket-remove HERE from the
>>>> source
>>>
>>> I disagree that this is a valid TODO (regardless of the version).
>>
>> It's meant to be a "just before the next branching point". We have
>> ~1500 calls to debugs() calls with HERE, ~3100 without. Maksing the
>> HERE symbol will make the effect go away, but it won't clean the
>> mess.
>> Such an invasive patch was rejected last week, so I added the TODO
>> to
>> remind to clean up the mess when the right moment in the release
>> cycle
>> will come.
>
> IMHO, there is no right moment for such an invasive patch (the
> reasons I
> discussed earlier will not change as the release cycle progresses).
> Others may overrule me on that, of course.

True. But until the code is consistent and clean of design defects we
are going to have to put up with periodic polishing bumps to remove
cruft.
IMO the best of the bad times for the big polishing is the gap between
X being made stable and X+1 branching to beta. When all of us have the
minimum of maintained branches to port across.

>
>>>> +#define HERE ""
>>>
>>> Have you checked that the compiler is smart enough to optimize this
>>> out?
>>> I suspect it is not. Please find a way to #define HERE so that the
>>> compiler can remove it. I recommend trying an inlined stream
>>> manipulator
>>> (that does not do anything).
>>
>> Ok, patch attached.
>
>> === modified file 'src/Debug.h'
>> --- src/Debug.h 2012-11-11 11:32:12 +0000
>> +++ src/Debug.h 2012-11-11 13:25:43 +0000
>> @@ -75,7 +75,6 @@
>>
>> class Debug
>> {
>> -
>> public:
>> static char *debugOptions;
>> static char *cache_log;
>
> Please remove the non-change in the first hunk.
>
>
>> + * HERE is a stream manipulatro which does nothing.
>> + * Its purpose is to inactivate calls made following previous
>> debugs()
>> + * guidelines such as
>> * debugs(1,2, HERE << "some message");
>> + *
>> + * The purpose previously held by HERE is now absorbed in the
>> debugs call itself
>> */
>
> Or you could just say "Legacy code. Do not use in new or updated
> code."
> or something like that :-).
>
> If you leave the detailed comment, please spellcheck "manipulatro".
>

or "/// \deprecated. Do not use. Remove on sight when editing old
code." which can be collated into this TODO list by doxygen:

   http://www.squid-cache.org/Doc/code/deprecated.dyn

Amos
Received on Mon Nov 12 2012 - 01:01:43 MST