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

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.

>>> +#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".

> +static inline std::ostream&
> +HERE(std::ostream& s)
> +{
> + return s;
> +}

If "static" is not needed, please remove that keyword.

Looks OK to me otherwise. Feel free to commit if there are no other
opinions. Can you tell whether GCC is optimizing HERE away after these
changes?

Thank you,

Alex.

>>> On 11/10/2012 08:47 AM, Amos Jeffries wrote:
>>> +1 from me.
>>
>>> On 11/10/2012 11:13 AM, Francesco Chemolli wrote:
>>> revno: 12436
>>
>> Would it be OK to add a submission process rule that discourages folks
>> from committing non-urgent changes as soon as they receive enough votes
>> for a commit? This may not be the best example, but there is a growing
>> number of cases where we have to adjust the freshly committed code
>> simply because there was not enough time for others to review it before
>> the commit?
>
> Merging soon small changes allows small-time developers such as myself
> to move on to the next topic sooner; it makes us more effective.
> Whether this can help or harm the project more, is up for discussion.
> I'd probably be the most affected by this policy, as I'm the one who
> focuses on these janitorial tasks the most.
>
> Kinkie
>
Received on Sun Nov 11 2012 - 19:25:11 MST