On Fri, 06 Aug 2010, Alexandr Ciornii wrote:
> 
> (It is beginning to feel colder in your room. It's a strange feeling,
> like someone is present here,  although you are alone... You now begin
> to hear some voices. It is some other person, from far, far away
> trying to tell something important to you.)
> 
> A shaman of other realm asked me to call to my gods of MSWindows realm
> for help. It required some time executing dark shaman rituals (at
> least 4 candies were sacrificed), and gods by method of tests and
> errors sent this solution to me. My mind is weak compared to gods one,
> so I can't understand this code, but godly presence makes this
> wonderful code work! I can only hope that this code will work in other
> realms too - my mind and body are too tired now to penetrate to other
> realm and test. I'll leave this task to you, shamans of other realms.
> 
> P.S. This patch has an important advantage - it works.
> P.P.S. Now I should sacrifice some more candies to regain my strength.

Against my better judgment I couldn't prevent myself from peeking at
the magical patch.  I don't understand what it is doing, and I don't
have any candies so sacrifice right now either.

However, I'm always suspicious when I see code like this:

    if ($^O =~ /win/i) { #Win32 dark magic. It works, so don't change anything

It isn't clear if that branch should be executed for Cygwin or not (currently
it will, although the comment sounds like it shouldn't, given that Cygwin is
considered to be a separate platform from Win32).  I prefer to always make 
things
explicit, either

    if ($^O eq "MSWin32" || $^O eq "cygwin") {

or

    if ($^O eq "MSWin32") {

Of course the "It works, so don't change anything" comment is another alarming
red flag: you should not make changes to code if you don't understand what the
changes are doing and/or if you can't explain why it does what it does.  Just
because you are getting rid of a symptom doesn't mean you solved the real issue.

I think it would be better to either SKIP or TODO a test instead of 
cargo-culting
a work-around that cannot be explained.

Cheers,
-Jan

Reply via email to