On 2009/03/18 18:36, Jochen Keil <jochen.k...@gmail.com> wrote: > please let me know about anything i should change or i could do better. > I've taken the parser from cuetools (you can find the outdated project > files on berlios) and modified the interface a bit.
I don't see what you modified. Did you add the original (unmodified) sources, or did you only add the versions which were already modified by you? The "clean" way to do it would be to create one commit which adds the original sources, and another commit edits them and adds them to the build system (Makefile.am). > I think it needs to be cleaned up a bit more but for i'm fine with. > It's not leaking memory anymore (at least i hope so) and i've added the > possibilty to delete an allocated cd structure. > > Hope you like it, I don't like it (yet)... see my comments below. > 67e65a7..0ce399a: > > commit 0ce399a344093caa24328971b51f74039597e749 > Author: Jochen Keil <jochen.k...@gmail.com> > > Makefile for the lexer/parser > > BSD style only for the moment. > It is easy to understand though if you want to compile > the test program or generate the lexer/parser. What's this for? > commit 755f16d9d0f09e11cea836e83d8ea47b002133f6 > Author: Jochen Keil <jochen.k...@gmail.com> > > Quick and dirty test program for the cuesheet parser > > Will not be compiled by mpd make Why not? Why not make it a test program in ./test/? This should live in test/, not in src/cue_parser/. If you don't compile it, don't add it to the source tree. > commit eebbd9a8043f301c312e3e6f63654c12314b4a70 > Author: Jochen Keil <jochen.k...@gmail.com> > > The implementation details for cue.h Who's the author? Why does it include "cd.h"? Why do we need "cd.h" at all? Looks like it's dealing with information for "compact discs", but MPD doesn't handle CDs. > commit 6fd0fa62fac30a90f5031940a8948731abe66c5d > Author: Jochen Keil <jochen.k...@gmail.com> > > Interface for tagging a song from a cuesheet > > commit ae70da3fbcbb0956e2a9c1b64aff5be0369c54a9 > Author: Jochen Keil <jochen.k...@gmail.com> > > Implementation details for time.h > > commit 507560abf378e84b450cab93c7f4ed9fe65882c7 > Author: Jochen Keil <jochen.k...@gmail.com> > > Methods for handling index times in cuesheet indices > > commit d05b0e8f44e7264d6e6021a7f6e56dec213c7414 > Author: Jochen Keil <jochen.k...@gmail.com> > > Implemenation for cdtext.h > > commit bc2cce8ed891db6b68ece6ef158251024617a781 > Author: Jochen Keil <jochen.k...@gmail.com> > > The CDText API and data struct > > commit 92dffa294d76ecccf273e66fccb40061d5bee2d7 > Author: Jochen Keil <jochen.k...@gmail.com> > > The implementation of the methods in cd.h Don't create a separate commit for each and every source file you import. cd.h is nothing without cd.c. > commit 41b7bcd0b4d8b5180fdf2fe70bb05a0d86adcac1 > Author: Jochen Keil <jochen.k...@gmail.com> > > The struct and the API for the cuesheet parser. > > commit 45403bcd4bbc2d86fca9951e855c673df8d8a44a > Author: Jochen Keil <jochen.k...@gmail.com> > > The Parser (grammar) H source file > > commit 0240445a630e6a1a520c2e0a13dc1f1081749c52 > Author: Jochen Keil <jochen.k...@gmail.com> > > The Parser (grammar) C source file > > commit 411bd222dfb1643237f4458753055c663959054d > Author: Jochen Keil <jochen.k...@gmail.com> > > The Lexer (Tokenizer) C source file See below for my comment on generated sources. > commit 74a52f5c2336ddde6623dc38752f7ca1523f937a > Author: Jochen Keil <jochen.k...@gmail.com> > > The Parser (grammar) source file > > commit 10f6fec9a7d816aac0a9f98ae2f4cac9751f0baf > Author: Jochen Keil <jochen.k...@gmail.com> > > The Lexer (Tokenizer) source file You're not the author of those files, yet the git commit reflects your name. I believe this is important. Anyway, what's the license of that source code? > commit d7d5ab14f3938b836d69d1124659e2fbb5ffeaf5 > Author: Jochen Keil <jochen.k...@gmail.com> > > Support for tagging flac files according to their embedded cuesheet > > commit 58ec6181c807b53de6d5a0e07e550a48c503c2fd > Author: Jochen Keil <jochen.k...@gmail.com> > > Added sources for the cuesheet parser This patch adds sources to Makefile.am which don't exist yet. This breaks the build on this patch - every commit should be buildable, or bisect won't work properly. Each commit which adds a new source must add it to Makefile.am. Each commit must not fail during build. "make distcheck" should always work, too. > Generating the lexer/parser using lex/yacc or flex/bison > isn't necessary every time for compiling. > That way we don't have to depend on those programs. I disagree. *Always* generate those sources. CUE support should be optional, so if lex/yacc are not available, don't build CUE support. We should never have generated files in git. Having yet another dependency isn't comfortable for users, but with this excuse, we could integrate everything into MPD. MPD used to have libmad built-in. ------------------------------------------------------------------------------ Apps built with the Adobe(R) Flex(R) framework and Flex Builder(TM) are powering Web 2.0 with engaging, cross-platform capabilities. Quickly and easily build your RIAs with Flex Builder, the Eclipse(TM)based development software that enables intelligent coding and step-through debugging. Download the free 60 day trial. http://p.sf.net/sfu/www-adobe-com _______________________________________________ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
