On Fri, Jan 16, 2009 at 12:17:33PM +0100, Thorkil Naur wrote:
> I have no opinion about this, I am not familiar with mapM or mapM_.
mapM_ is to mapM what mapc is to mapcar (in Lisp): mapM collates
results, but as they are then simply discarded above, it makes sense
to use mapM_, which does not collate results in the first place.
i.e. mapM_ is clearly called ONLY for "side effects", never for its
result which is always (I assume) the null value ().
> I never use concatMap myself. I seem to recall somebody expressing
> the opinion that including concatMap as part of the fundamental
> arsenal of functions was a mistake and I wouldn't disagree. But,
> again, this is very much a matter of taste and I don't consider the
> use of concatMap wrong in any sense.
Well surely concatMap *isn't* fundamental, any more than the Y
combinator (it's defined in terms of U). IMO that doesn't invalidate
its use a convenient shorthand, just as one might use contractions
like "can't" instead of "cannot" in informal English.
I'd quite like the concatMap patch to be applied, because personally
when I look at concatMap I can see the idiom that's being expressed
more clearly than if map and concat are applied separately.
> > [Use one map.
> > hlint**20090115033047
> > Ignore-this: 6ba09e739f2f1053c86e21d67df73fcd
> > ] hunk ./src/Darcs/Commands/SetPref.lhs 69
> > "\n" ++
> > "Valid preferences are:\n" ++
> > "\n" ++
> > - (unlines $ map unwords $ map (\ (x,y) -> [" ",x,"--",y]) valid_pref_data)
> ++
> > + (unlines $ map (\ (x,y) -> unwords [" ",x,"--",y]) valid_pref_data) ++
> > "\n" ++
> > "For example, a project using GNU autotools, with a `make test' target\n"
> ++
> > "to perform regression tests, might enable Darcs' integrated
> > regression\n"
> ++
>
> I cannot easily understand either of these variants.
What it does is turn a list of pairs [(name :: String, description ::
String)] into a single string by prefixing the name with a space,
sticking an en dash in front of the description, then joining words
(unwords) and lines (unlines).
I *was* accumulating a list of words, then mapping unwords over them.
It makes more sense to unword them right at the start.
Of course, the following would be far better:
unlines [" "++x++" -- "++y | (x,y) <- valid_pref_data]
Hopefully I won't forget to submit that as a patch :-)
> > [Use (:).
> > hlint**20090115033119
> > Ignore-this: db2b1893adc4c9b6c1e09b17c0031c5a
> > ] hunk ./src/Darcs/Arguments.lhs 1207
> > show_short_options a ++ show_long_options b ++ latex_help h ++ "\\\\"
> > option_latex (DarcsArgOption a b _ arg h) =
> > show_short_options a ++
> > - show_long_options (map (++(" "++arg)) b) ++ latex_help h ++ "\\\\"
> > + show_long_options (map (++(' ':arg)) b) ++ latex_help h ++ "\\\\"
> > option_latex (DarcsAbsPathOrStdOption a b _ arg h) =
> > show_short_options a ++
> > hunk ./src/Darcs/Arguments.lhs 1210
> > - show_long_options (map (++(" "++arg)) b) ++ latex_help h ++ "\\\\"
> > + show_long_options (map (++(' ':arg)) b) ++ latex_help h ++ "\\\\"
> > option_latex (DarcsAbsPathOption a b _ arg h) =
> > show_short_options a ++
> > hunk ./src/Darcs/Arguments.lhs 1213
> > - show_long_options (map (++(" "++arg)) b) ++ latex_help h ++ "\\\\"
> > + show_long_options (map (++(' ':arg)) b) ++ latex_help h ++ "\\\\"
> > option_latex (DarcsOptAbsPathOption a b _ _ arg h) =
> > show_short_options a ++
> > show_long_options (map (++("[="++arg++"]")) b) ++ latex_help h ++
> "\\\\"
>
> I have to break off here and comment on the above hunks separately: The code
> is clearly about constructing character strings to be displayed to the human
> user. So performance is not a particularly pressing issue. In such a setting,
> matters are considerably eased, in my opinion, if you just decide to say,
> well, everything is a string and they are combined using ++. Instead of
> having to think carefully about each individual concatenation, could this now
> be expressed in a more compact or efficient manner. If you do the latter, the
> code becomes more difficult to change, for example, if you decide to extend
> one of the strings of length 1 that is incidentally represented as a
> character constant.
I really don't see it as a big deal to go from 'x':y to "px"++y and
back again. Perhaps my Lisp background just makes me see adding an
element to a list with LIST and APPEND as needlessly circuitous.
I don't care enough to push for this patch -- as you say, the
performance burden is minimal.
> > Use const.[...]
> > -normalRegChars [] = \_ -> False
> > +normalRegChars [] = const False
>
> I rarely use const myself, so I think the originals read clearer than the
> modified ones.
Shrug; again I don't care enough to push the point.
> > [Use replicate.
> > hlint**20090115033241
> > Ignore-this: 9802026b0051d4ab6377b75ce6a453f4
> > ] hunk ./src/Crypt/SHA256.hs 30
> >
> > sha256sum :: B.ByteString -> String
> > sha256sum p = unsafePerformIO $
> > - withCString (take 64 $ repeat 'x') $ \digestCString ->
> > + withCString (replicate 64 'x') $ \digestCString ->
> > unsafeUseAsCStringLen p $ \(ptr,n) ->
> > do let digest = castPtr digestCString :: Ptr Word8
> > c_sha256 ptr (fromIntegral n) digest
> > hunk ./src/Darcs/Repository/Cache.hs 78
> > cacheHash :: B.ByteString -> String
> > cacheHash ps = case show (B.length ps) of
> > x | l > 10 -> sha256sum ps
> > - | otherwise -> take (10-l) (repeat '0') ++ x
> ++'-':sha256sum ps
> > + | otherwise -> replicate (10-l) '0' ++ x
> > ++'-':sha256sum
> ps
> > where l = length x
> >
> > okayHash :: String -> Bool
>
> These changes make sense. There even seems to be more to be done
> here: The repeated "10", for example, couldn't that be avoided,
> somehow?
I don't see how; it's adding padding out to a certain length, so it
needs to know both that length and the length of the part that doesn't
need padding. i.e. it's doing sprintf ("%010d",x)
> Nevertheless, here we find ourselves in areas (Crypt/SHA256.hs and
> Darcs/Repository/Cache.hs) that seem extremely delicate,
That's why we have lots of regression tests and a sturdy release
manager :-)
> So this is really a case of "If it ain't broken, don't fix it" in my
> view.
I guess I consider code that's ugly or unclear to be "broken"
just as much as if it is slow or produces the wrong result :-)
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
