----- Original Message ----- From: "Robert Collins" <[EMAIL PROTECTED]> To: <[EMAIL PROTECTED]> Sent: Friday, February 01, 2002 13:58 Subject: For the curious: Setup.exe char-> String patch
> This is what I'm preparing to commit. I'm mailing it here as a preview, > because doing the changelog is going to take.... some time. (160Kb diff > to wade through). > > The cstr_oneuse() is designed to allow efficient (via caching, not > implemented yet) leak-safe char * usage of a string. An equivalent > wcstr_oneuse() can be implemented to expose a wide char variant. (The > underlying encoding of the string class will eventually be UTF-8/UTF-16 > or something similar). > > I haven't implemented substrings yet, which is why the large number of > .cstr_oneuse() calls and a few non-converted routines. > > Anyway, it's all working for me: ]. I only saw two possible real problems. Everything else is a matter of consistency which I could send you a patch for after this is implemented. 1. In String++.cc, I think 0 may be returned for a character in the first position since the for loop starts with i=0. 2. In geturl.cc, I think you are deleting a char[] after changing the pointer. String++.cc: +// does this character exist in the string? +// 0 is false, 1 is the first position... +size_t +String::find(char aChar) const +{ + for (size_t i=0; i < theData->length; ++i) + if (theData->theString[i] == aChar) + return i; + return 0; ### Won't this return 0 if aChar is in the first position in theData->theString? String++.h: ### Do you want String::concat() and String::vconcat to be public? The few places I see them being used could be ... String ("first") + next + next ... You lose a little efficiency by not calling String::concat, but you make up some of it by not having to call String::cstr_oneuse(). archive.cc: - if (io_stream::mkpath_p (PATH_TO_FILE, destfilename)) - {log (LOG_TIMESTAMP, "Failed to make the path for %s", destfilename); + if (io_stream::mkpath_p (PATH_TO_FILE, destfilename.cstr_oneuse())) + {log (LOG_TIMESTAMP, "Failed to make the path for %s", destfilename.cstr_oneuse()); ### To be consistent with other log() calls in this file change the last line to: + {log (LOG_TIMESTAMP, String ("Failed to make the path for ") + destfilename); - if (io_stream::mkpath_p (PATH_TO_FILE, destfilename)) - {log (LOG_TIMESTAMP, "Failed to make the path for %s", destfilename); + if (io_stream::mkpath_p (PATH_TO_FILE, destfilename.cstr_oneuse())) + {log (LOG_TIMESTAMP, "Failed to make the path for %s", destfilename.cstr_oneuse()); ### To be consistent with other log() calls in this file change the last line to: + {log (LOG_TIMESTAMP, String ("Failed to make the path for ") + destfilename); desktop.cc: - fname = concat (path, "/", title, ".pif", 0); /* check for a pif as well */ + fname = String::concat (path, "/", title.cstr_oneuse(), ".pif", 0); /* check for a pif as well */ ### To avoid exposing String::concat() change last line to: + fname = String (path) + "/" + title + ".pif"; /* check for a pif as well */ - fname = concat (path, "/", title, ".pif", 0); /* check for a pif as well */ + fname = String::concat (path, "/", title.cstr_oneuse(), ".pif", 0); /* check for a pif as well */ ### To avoid exposing String::concat() change last line to: + fname = String (path) + "/" + title + ".pif"; /* check for a pif as well */ download.cc: + log (LOG_TIMESTAMP, "Downloaded %s", local.cstr_oneuse()); ### To minimize the use of log( level, format, ....), change the line to: + log (LOG_TIMESTAMP, String ("Downloaded ") + local); geturl.cc: static void -init_dialog (char const *url, int length, HWND owner) +init_dialog (String const url, int length, HWND owner) { if (is_local_install) return; - char const *sl = url; + char const *sl = url.cstr(); char const *cp; - for (cp = url; *cp; cp++) + for (cp = sl; *cp; cp++) if (*cp == '/' || *cp == '\\' || *cp == ':') sl = cp + 1; max_bytes = length; @@ -71,6 +74,7 @@ init_dialog (char const *url, int length Progress.SetText3("Connecting..."); Progress.SetBar1(0); start_tics = GetTickCount (); + delete[] sl; } ### Is that delete[] safe? You've been changing sl repeatedly in the for loop. - log (LOG_BABBLE, "get_url_to_membuf %s", _url); + log (LOG_BABBLE, "get_url_to_membuf %s", _url.cstr_oneuse()); ### To minimize the use of log( level, format, ....), change the line to: + log (LOG_BABBLE, String ("get_url_to_membuf ") + _url); - log (LOG_BABBLE, "get_url_to_file %s %s", _url, _filename); + log (LOG_BABBLE, "get_url_to_file %s %s", _url.cstr_oneuse(), _filename.cstr_oneuse()); ### To minimize the use of log( level, format, ....), change the line to: + log (LOG_BABBLE, String ("get_url_to_file ") + _url + " " + _filename); localdir.cc: - log (0, "Selected local directory: %s", local_dir); - if (SetCurrentDirectoryA (local_dir)) + log (LOG_TIMESTAMP, "Selected local directory: %s", local_dir.cstr_oneuse()); + if (SetCurrentDirectoryA (local_dir.cstr_oneuse())) ### To minimize the use of log( level, format, ....), change the line to: + log (LOG_TIMESTAMP, String ("Selected local directory: ") + local_dir); main.cc: + log (LOG_TIMESTAMP, "Current Directory: %s", cwd); ### To minimize the use of log( level, format, ....), change the line to: + log (LOG_TIMESTAMP, String ("Current Directory: ") + local_dir); log.cc/log.h: ### If I understand the change, a log line may be either timestamped or babble. So a line can't be timestamped and only go to setup.log.full. Likewise all lines in setup.log must be timestamped. I think we are losing some useful capablities by changing from flags to level. mount.cc: ### It looks like it might be cleaner to make String cygpath (String const &) visible along with String cygpath (const char *, ...). It seems like nearly every place I saw it used you are doing cygpath (xxx.cstr_oneuse(),0). ### The few places that involve concatenation could be handled by String()+x+... I'm willing to make a patch to catch any leftovers so String cygpath( const char *, ...) could be dropped. package_meta.cc: - log (LOG_BABBLE, "unlink %s", d); - SetFileAttributes (d, dw & ~FILE_ATTRIBUTE_READONLY); - DeleteFile (d); + log (LOG_BABBLE, String("unlink ")+ d.cstr_oneuse()); + SetFileAttributes (d.cstr_oneuse(), dw & ~FILE_ATTRIBUTE_READONLY); + DeleteFile (d.cstr_oneuse()); ### I don't think .cstr_oneuse() is needed for log(): + log (LOG_BABBLE, String("unlink ") + d); site.cc: - log (0, "site: %s", site_list[n]->url); + log (LOG_TIMESTAMP, "site: %s", site_list[n]->url.cstr_oneuse()); ### To minimize the use of log( level, format, ....), change the line to: + log (LOG_TIMESTAMP, String ("site: ") + site_list[n]->url); - log (0, "Adding site: %s", other_url); + log (LOG_BABBLE, "Adding site: %s", other_url.cstr_oneuse()); ### To minimize the use of log( level, format, ....), change the line to: + log (LOG_BABBLE, String ("Adding site: ") + other_url); source.cc: - log (0, "source: %s", + log (LOG_TIMESTAMP, "source: %s", ### To minimize the use of log( level, format, ....), change the line to: + log (LOG_TIMESTAMP, String ("source: ") + -- Mac :}) ** I normally forward private questions to the appropriate mail list. ** Ask Smarter: http://www.tuxedo.org/~esr/faqs/smart-questions.htm Give a hobbit a fish and he eats fish for a day. Give a hobbit a ring and he eats fish for an age.