[Kstars-devel] Submitting my first patch and saying hello

[Andrew Buck]

Hello everyone, this is my first post to this mailing list, in fact the
first to any "dev" mailing list at all, so forgive me (and please
correct me) if I my message contains some sort of a problem.  I would
like to begin by thanking all of you for making this wonderful program,
I have learned more about the motions of the stars/planets your program
than from anywhere else, and I have only been using it a short time.

The primary reason for my post is I would like to submit a bit of a
patch, I hope to do more substantial work in the future but I haven't
learned too much about the code base yet so I am just "testing the
waters" with what I think might be a bit of a performance improvement.
I run a debian system so I grabbed a copy of the source code from
"apt-get source" and compiled it with deugging and profiling turned on.
After letting it crunch away at a time advance of 5 minutes to keep the
CPU loaded, I examined the grpof output and the biggest "self time" was
the function dms::setFromString(...) defined in dms.cpp.  Upon
examination of the function I found this block of code...

	QString entry = str.stripWhiteSpace();

	//remove any instances of unit characters.
	//h, d, m, s, ', ", or the degree symbol (ASCII 176)
	entry.replace( QRegExp("h"), "" );
	entry.replace( QRegExp("d"), "" );
	entry.replace( QRegExp("m"), "" );
	entry.replace( QRegExp("s"), "" );
	QString sdeg;
	sdeg.sprintf("%c", 176);
	entry.replace( QRegExp(sdeg), "" );
	entry.replace( QRegExp("\'"), "" );
	entry.replace( QRegExp("\""), "" );

I wrote a replacement for the block which is the following...

	QString entry;

	//loop over the string passed to us and remove unwanted
	//characters, storing the cleaned up string in 'entry'
	for(int i = 0; i < str.length(); i++) {

		//if character is one of [h, d, m, s, ', ", the degree
		//symbol (ASCII 176), or whitespace] skip over it (i.e. do
		//nothing) else append it to entry
		switch(str[i].latin1()) {
			case 'h':
			case 'd':
			case 'm':
			case 's':
			case '\'':	//single quote (apostrophe)
			case '\"':	//double quote
			case 176:	//degree symbol
			case 9:		//tab
			case 10:	//linefeed
			case 11:	//vertical tab
			case 12:	//form feed
			case 13:	//carriage return
			case 32:	//space
			break;

			default:
			entry += str[i];
		}
	}

The new block of code should perform the same function without the
overhead of calling any functions.  Adittionaly, it only loops over the
string once, instead of having to (presumably) perform a loop for every
"entry.replace(...)" call.  I was unable to test the function
explicitely as I don't know enough about the rest of the code base to
really put it through its paces, but the modified program does compile
and execute (properly as far as I can tell).  In addition to this gprof
says it is no longer the biggest time user (second biggest now so some
more optimization might be in order) so it does appear to be a
successful optimization.

The lines that cut out whitespace in the switch statement are there
because of the "QString entry = str.stripWhiteSpace();" found in the
original, but I see the SVN version now has "QString entry =
str.trimmed();" instead.  I was unable to find the "trimmed" functon in
the QString class documention I found online so I'm not sure what it
does but I would expect it just removes whitespace or some other set of
characters, if this is the case then the case statements in the switch
statement can just be modified to reflect the new behaviour.  I am
actually a little bit confused by the original as I would think removing
whitespace seperating degrees, minutes, and seconds would reek havoc on
trying to parse out the values later, but maybe not.  Anyway as I
mentioned the code does appear to perform properly but someone more
familiar with the code base should have a look before it goes into the
SVN version.

I am sorry for not being able to provide a proper diff of the change but
the SVN version seems to be different enough from the debian source
package I worked from to confuse diff into thinking the whole file
changed.  In the future I will grab myself an SVN copy so I can submit a
diff as is normally done.

Again thank you for this wonderful program, and let me know if there is
anything I should be doing differently in the future when I submit
patches, etc.

-Buck