This is the mail archive of the
cygwin-apps
mailing list for the Cygwin project.
Re: [PATCH setup] site.cc, site.h: code cleanup
- From: Jon Turney <jon dot turney at dronecode dot org dot uk>
- To: cygwin-apps at cygwin dot com
- Cc: Ken Brown <kbrown at cornell dot edu>
- Date: Wed, 22 Nov 2017 17:02:12 +0000
- Subject: Re: [PATCH setup] site.cc, site.h: code cleanup
- Authentication-results: sourceware.org; auth=none
- References: <20171120012615.10428-1-kbrown@cornell.edu>
On 20/11/2017 01:26, Ken Brown wrote:
Remove site_list_type::init(), which was introduced to work around a
problem with gcc-2.95.
Please tell me we're not actually using a placement new for these things :S
Add a bool member 'is_official' to the site_list_type class. Use it
to distinguish official mirrors (listed in mirrors.lst) from
user-added sites. This replaces the (undocumented) use of
site_list_type::servername.size() for this purpose.
When registerSavedSite is called on a URL that's already in
'all_site_list', add the version from 'all_site_list' to 'site_list'
rather than adding a temporary version that contains no information
other than the URL.
Similarly, if the user adds a site that was already in
'all_site_list', don't replace the existent version with the new one
(which contains only the URL).
This is a nice bit of cleanup.
My only concern is with the terminology "is_offical".
In general, this code suffers from confusion between (i) package
repository [a set of packages], and (ii) mirrors [a set of sites
offering the same package repository]
(See also the musings in [1]. I have a vague recollection that I
actually started writing some code to do some of that, but that would be
in an old CVS checkout somewhere)
We have the concept of a label for the package set ('release:' in
setup.ini [2])
To me "is_official" gives the idea that this flag means "the package set
at this URL is the package set with the label 'cygwin'", but actually it
just means "this URL was read from mirrors.lst", so maybe it could be
named something like that?
Oh, you actually have a comment to that effect...
[1] https://cygwin.com/ml/cygwin-apps/2011-04/msg00014.html
[2] https://sourceware.org/cygwin-apps/setup.ini.html