Comments on patch (which probably should have gone to cfe-commits or 
Phabricator):

+# portability: convert POSIX cygwin path to MS-DOS style path

Please start comments with a capital letter.


+  $PortablePath =~ s/^\/cygdrive\/(\w)/$1:/o if ($^O =~ /cygwin/);

The user is free to modify the mount path for /cygdrive/, but I guess that's 
okay. What I'm more confused about is how programs running inside Cygwin are 
supposed to use Windows-ish paths (still with forward slashes) to run 
executables. But I don't have a Windows machine available, so I'll trust you 
know what's right here. Please do try to hand-test all the code paths affected 
by this if you haven't already.


+# portability: getpwuid is not implemented for Win32 (see Perl language 
reference, perlport)
+my $UserName = ($^O =~/MSWin32/) ? HtmlEscape(getlogin || 'unknown')
+                                 : HtmlEscape(getpwuid($<) || 'unknown');

How about just getlogin() || getpwuid($<) || 'unknown'? That seems to be fairly 
idiomatic in a quick search online.

(This is only used for metadata in the generated output, so it's not even that 
important.)


+print "NewDir = ".$NewDir."\n";

Stray debug print?


+# portability: use less strict but portable check -e (file exists) instead of 
+# non-portable -x (file is executable). On some windows ports -x just checks
+# file extension to determine if a file is executable (see Perl language 
reference, perlport)

Capital letter, and we should also follow the 80-column limit going forward.

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to