On Wed, Dec 19, 2007 at 10:00:48AM -0800, Terminator wrote:
> > I have a simple question. I often meet in mrxvt's code some parts
> > where there is maybe not an error but a goofiness. For instance some
> > actions are made several times unecessary (eg. a test on a buffer to
> > check whether an action has to be made whereas there is no
> > possibility that this buffer has been modified since last time we
> > checked!), etc. Other times, some variables are set several times in
> > a row to the same value (but this may be "hidden" in the code), or
> > it is set and immediately set to some other value, etc.
> >
> > I am not always 100% sure, and I would need to make more extensive
> > tests. Some other times this is obvious. As an example yesterday (or
> > 2 days ago, I am not sure), I saw a loop which define variables,
> > always at same value (so independant to the incremental variable).
> > The logic would be to simply get this variable definition out of the
> > loop because this is unecessary variable creation (I guess each loop
> > would create a different variable, unless the compiler can "see" it
> > and optimize, but even though, I find it not good programming). This
> > was in rxvt_scr_expose (screen.c):
> >
> > for (i = ...; ...; i++)
> > {
> > register int j = rc[PART_BEG].col;
> > register int k = rc[PART_END].col - rc[PART_BEG].col + 1;
> > MEMSET (&(PVTS(r, page)->drawn_text[i][j], 0, k);
> > }
> >
> > here I would obviously get j and k def out of the loop.
>
> The above code is perfectly find. Defining j and k within the for {}
> block ensures that they will not be used outside of the code block.
> Such arrangement has a benefit: restrict the scope of variables to
> avoid accidental modification/usage at somewhere else. It improve the
> quality of the code actually. Of course, you can move them outside of
> the block. But I do not think it is necessary.
I agree with Jimmy on this one: A good compiler should be able to
optimize such things (I hope). It's more important for humans to keep
the code readable :).
If you're worried, you could change it to something like
{
int j = ...
int k = ...
for(...)
{
...
MEMSET(...)
}
}
(i.e. Define j, k outside the loop, but scope them inside "{" and "}".)
This might make things a little faster, while still keeping the code
readable.
Again, I'm not sure if the optimized code will be any different though!
(But then again, I'm not a CS person and don't know the workings of
gcc).
> > The question is then:
> >
> > when I see "details" like this, can I change it and commit just for
> > this? Or do you want only big and meaningful commits? Because I
> > write them on my notebook but I fear to forget them all the same
> > (and as I run tests, I often reset my local code to original state).
Small commits (like the one above) are totally fine. In fact they're
preferable! That way if you break something, one can do a "binary
search" to figure out which revision the problem is with (A kind user
did this with a bug I introduced once...). If the changes are small,
then the problem should be easy to isolate and fix.
Just try and make sure that the code you commit compiles, and preferably
doesn't break current features... :). Be verbose with your log messages.
And commit away :).
GI
--
Twenty Ways To Maintain A Healthy Level of Insanity
4. Put your garbage can on your desk and label it "In".
-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
Materm-devel mailing list
Materm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/materm-devel
mrxvt home page: http://materm.sourceforge.net
