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

[Luciano Montanaro]

Il Tuesday 04 December 2007 08:41:56 Andrew Buck ha scritto:
> 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];
> 		}
> 	}
>

Hi, and welcome. Although, I'm mostly an intersted user...
I don't know where the code is used, but the original code is actually 
suboptimal...

Yours seem for sure an improvement. I don't know if it's executed in any 
critical path (but I guess so, from what you write), but the same, if I 
understand correctly, the string cleanup could also be written like:

entry.remove("[hdms'\"\176 \t\n\r\v\f]");

Although the '\176' is probably wrong. That's the latin1 code for the degree 
symbol, and qstring expects an Unicode char. 

entry.remove("[hdms'\"° \t\n\r\v\f]");

should work, though.

Luciano