This is the mail archive of the cygwin-patches@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: For the curious: Setup.exe char-> String patch


----- Original Message -----
From: "Robert Collins" <robert.collins@itdomain.com.au>
To: <cygwin-patches@cygwin.com>
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.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]