On 2013/12/17 18:08, j...@dockes.org wrote:
> Max Kellermann writes:
> > I see many bug fix commit, which fix bugs introduced earlier by
> > yourself. This is confusing to read, and makes the repository hard to
> > bisect. Please fold those into the commit that introduced the bug.
>
> Ok, then, I've squashed everything into one commit, as the history was
> all bug fixes over the initial code add.
Please fix indentation. You mix tabs and spaces, and obviously your
tab width is not 8. Also, please remove all whitespace at the end of
code lines.
Use pkg-config instead of AC_CHECK_LIB. That's more reliable and
easier to use.
+static const string rootid("0");
Using std::string here is useless bloat, isn't it? There's much more
of this useless bloat in your code.
Use nullptr instead of 0. 0 is obscure.
Don't use "m_" prefixes for variable names. Put class variables on
top, not at the bottom, just like the rest of the C++ code does.
+static void stringToTokens(const string& str, vector<string>& tokens,
+ const string& delims = "/", bool skipinit = true)
This should return the vector instead of passing a writable reference.
Using reference parameters for return values is obscure. And I think
this function should live in src/util/.
Passing "delims" and "str" as std::string is useless bloat, again.
What is reallyOpen()? The method name is obscure.
+ if (m_root)
+ return true;
What does that check mean?
+ FormatDebug(db_domain, "UpnpDatabase::reallyOpen\n");
Do we need those debug messages? What value do they add?
+ // Wait for device answers. This should be consistent with the value set
+ // in the lib (currently 2)
+ sleep(2);
No, no, no. I had a hard time removing all those sleep() calls from
MPD, and I'm not even finished - I will not allow adding new ones.
This is extremely bad style. Completely not acceptable. Blocking is
bad enough, but waiting for a compile-time constant amount of time for
some event to happen, without knowing how long it will really take -
no!
+ // TBD decide what we do with the lib and superdir objects
What does that mean?
+// Transform titles to turn '/' into '_' to make them acceptable path
+// elements. There is a very slight risk of collision in doing
+// this. Twonky returns directory names (titles) like 'Artist/Album':
Please use doxygen-style comments for API documentation. Some more
API documentation would be nice.
+static map<string, TagType> propToTag = {
+static map<unsigned int, string> tagToProp = {
Bloat! Use tag_table instead.
+ std::ostringstream oss;
+ oss << "Unknown tag value " << tagnum;
+ return oss.str();
That code should not be reachable. Ensure that and convert to
assert(). But, anyway:
+// Debug/printing: translate MPD tag value to string
+// Debug/printing: print out a SongFilter
Do we really need that?
+/* Local Variables: */
+/* mode: c++ */
+/* c-basic-offset: 4 */
+/* tab-width: 4 */
+/* indent-tabs-mode: t */
+/* End: */
Here it is: tab-width 4. Please remove this section and correct your
editor configuration!
+ * ExpatMM - C++ Wrapper for Expat available at http://expat.sourceforge.net/
Please not another code duplication.
Is this C++ wrapper really worth it? I use plain expat, even in C++.
Wrapping expat in a C++ interface just adds bloat with no advantage.
+class PTMutexInit {
MPD already has a Mutex class, one that is portable. Why do we need a
new one, one that is not portable?
+extern std::string path_getfather(const std::string &s);
MPD already has code for dealing with path names. No more code
duplication!
+#define PLOGINF(...) UpnpPrintf(UPNP_INFO, API, __FILE__, __LINE__,
__VA_ARGS__)
+#define PLOGDEB(...) UpnpPrintf(UPNP_INFO,API, __FILE__, __LINE__, __VA_ARGS__)
Another logging library? MPD already has one.
+#include <sstream>
Referencing iostream in MPD adds a huge amount of bloat (even without
actually using it). We don't use it currently, and I don't see any
sensible use of it in your commit. Please remove.
+ LibUPnP(const LibUPnP &) {}
+ LibUPnP& operator=(const LibUPnP &) {return *this;};
W T F. An assignment operator/constructor that does nothing. That is
insane code!
+#include <tr1/unordered_map>
+#include <tr1/unordered_set>
Don't use TR1 headers. We have C++11, and that's part of the standard
now.
+ if ((err = pthread_create(&thr, 0, workproc, arg))) {
That code is not portable. Please use MPD's portable threading
library.
------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT
organizations don't have a clear picture of how application performance
affects their revenue. With AppDynamics, you get 100% visibility into your
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
_______________________________________________
Musicpd-dev-team mailing list
Musicpd-dev-team@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
