This is the mail archive of the cygwin-apps 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: Command line argument to setup.exe for a package list to install


Corinna Vinschen wrote:

> > Here's the patch that my colleague did:
> > http://www.tcm.phy.cam.ac.uk/~mr349/cygwin.patch
> > It is definitely not the best code, but it works and meets our needs and
> > maybe will be helpful to the cygwin project.
> 
> Wrong mailing list.  setup.exe is handled entirely on cygwin-apps.
> See http://cygwin.com/lists.html

I have no objection to the spirit of this patch, however:

- The formatting does not follow the GCS, for example:

+bool
+packagemeta::isManuallyWanted() const
+{  
+  string packages_option = PackageOption;
+  string tname;
+  /* Split the packages listed in the option up */
+  string::size_type loc = packages_option.find( ",", 0 );
+  bool breturn=false;
+  while ( loc != string::npos ) {
+    tname=packages_option.substr(0,loc);
+    packages_option=packages_option.substr(loc+1);
+    breturn = breturn || (name.compare(tname)==0);
+    loc = packages_option.find( ",", 0 );
+  }
+  /* At this point, no "," exists */
+  breturn=breturn || (name.compare(packages_option)==0);
+  return breturn;
+}
+

The whitespace and brace style is inconsistent.  There should always be
space on either side of "=" and before "("; the opening and closing
braces go on their own lines; there should be a period and two spaces at
the end of a comment; etc.

- There was no ChangeLog included.

- Please not include changes to generated files such as setup_version.c.

- Adding a new command line option causes the FAQ to be out of date
(http://cygwin.com/faq/faq.setup.html#faq.setup.cli) so please submit a
separate patch to update the FAQ as well.

Brian


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