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

Robert Collins robert.collins@itdomain.com.au
Fri Feb 1 18:45:00 GMT 2002



> -----Original Message-----
> From: Michael A Chase [mailto:mchase@ix.netcom.com]
> 
> 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.
>
> 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?

Yes. Thank you. 
 
> 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().

HMMM, worth thinking about. Remeber that vconcat can only be used with
char *'s, and we don't want them :}. (think unicode native storage).
There are other lower lever mechanisms to optimise String, but as we
aren't CPU bound, I'm not concerned at this point. One such example you
could look at is the SGI Rope class template. (I've not looked at that
but it's similar to what another project I'm on has been creating from
scratch, a team member recently said "hey, this is similar :}". As for
concat vs +, concat canonicalises paths, which is what broke Chuck's //
path references (because / indicated a posix path to the code AFAIK). I
don't think thats an appropriate use for String:: though, so wrote +,
and used that. Also, canonicalisation IMO should occur at the
io_stream::open and related calls, not at every manipulation: file path
fragments shouldn't get canonicalised.
 
> archive.cc:

> ### 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);

...

Yah, it's not 100% complete :]. Let me get it in first, then we can all
chip in an tidy up :}.
 
> geturl.cc:
> 
>  static void
> -init_dialog (char const *url, int length, HWND owner)
> +init_dialog (String const url, int length, HWND owner)

>    start_tics = GetTickCount ();
> +  delete[] sl;
>  }
> 
> ### Is that delete[] safe?  You've been changing sl 
> repeatedly in the for
> loop.

I'll check when I get home. I may have missed it - I've used a temp
variable most other places. Thanks for the catch (assuming it's a bug
:]).
 
> 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.

mmm, yes, but we've also gained type safety. If you wish to submit a
flags class ( I can enlarge on that if needed) to allow log
(LOG_BABBLE|LOG_FULL & LOG_TIMESTAMP|LOG_LITERAL, String const &) that'd
be fine by me. I like enforcing type safety where possible.
 
> 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).

Yes, I want to... but doing it was going to be a right ol' pain
initially, so I pu tit to the side.
 
> ### 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.

Please do.
 
> package_meta.cc:

> ### I don't think .cstr_oneuse() is needed for log():
> +       log (LOG_BABBLE, String("unlink ") + d);

another good catch.
 
Thanks very much for the detailed analysis. 

Rob



More information about the Cygwin-patches mailing list