Re: [Musicpd-dev-team] UPnP db plugin
On 2013/12/18 10:02, j...@dockes.org wrote: +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. I do not think that this is useless bloat, except if you consider that std::string is useless bloat in general. As most of the code is based on std::string, defining the constant as such means that we do not incur a char*-to-std::string conversion each time the constant is used (to construct another string). When would this conversion happen? Use nullptr instead of 0. 0 is obscure. 0 is not obscure, as it was the standard up to really recently (NULL usage was discouraged by Stroustrup himself, you can easily google for this). It WAS the standard. MPD was not designed for this outdated standard. It was rewritten to C++11. When MPD was C99, it used NULL - Stroustrup's opinion does not matter here, because he was talking about C++. Now MPD uses C++11, and it shall use nullptr - Stroustrup's opinion still does not matter, because it's obsolete. I am all for converting to nullptr, but saying that 0 is obscure is a bit of an exaggeration. It obscures the nature of the variable. Using 0 implies that it's an integer, but it's not. It was obscure even when it was the standard. This old standard is obsolete for a good reason. I can change the declaration order if you really insist, but you should be aware that many people (e.g. Google standards) quite logically recommend to put the public declarations (the interface) first, and the private stuff (including the data members) last because this improves readability. There are arguments both for both sides. It boilds down to personal opinion. There are no logical super-arguments for any. I criticized your coding style because it does not follow what the rest of MPD does. Google standards are irrelevant here. The variables are not passed as std::string but as const refs to std::string, so, no overhead. Const or not const does not matter for the question of whether it's overhead, does it? It is overhead because a simple string literal must be converted to std::string each time the function is called - and that implies a heap allocation and deallocation. I actually don't understand your remark. Using std::string for everything except when constrained by an external interface is just good c++ practise. Maybe in your opinion. It's not in my opinion. MPD uses C++ syntax because it is easier to write than C. I converted MPD from C99 to C++ when C++11 came, which allowed me to eliminate most of the runtime overhead. Only then was C++ acceptable for me. Now you're adding all the C++ bloat that I feared, why I have been disliking C++ for so many years. Using a writable ref for returning the result avoids copying a possibly expensive object. This is also just good practise and I think that it's not clear to you because you are not used to it: non-const refs are for returning data, else they would be const... You may want to read https://en.wikipedia.org/wiki/Return_value_optimization What is reallyOpen()? The method name is obscure. I have to delay initialization of libupnp until after mpd daemonizes, else the lib does not work. I don't know if the fork does it or some descriptor manipulation. So it's a kludge to work around a MPD initialization order problem. This should be well-documented in a code comment so it can be eliminated once the MPD core has been improved. And you should write a ticket to request this improvement. + if (m_root) + return true; What does that check mean? As reallyOpen() has to be called for every visit call this is checking if we need to actually do something or we're already done. Setting m_root is the last thing the method does when really working. I could put the test at the call locations, but I find it cleaner this way. Again, there should be a code comment explaining this kludge. + FormatDebug(db_domain, UpnpDatabase::reallyOpen\n); Do we need those debug messages? What value do they add? Well, they're used for debugging actually. I could remove them, but having traces for the main function calls is really useful. I can ifdef all this away if you prefer. No. Please remove them completely. I'm not convinced that they are really useful. They just clutter the log file. + // 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! When the UPnP discoverer inside the
Re: [Musicpd-dev-team] UPnP db plugin
You know what: you are right. The UPnP plugin is 4000 lines of bloat and your nitpicking about a few hundred lines of commodity code inside it has just saved you from the whole packet. A few months ago, I put up with your flat out lies about the Proxy DB plugin (no denying possible, just see email messages and commit log), and I've answered calmly to your abuse from last night, because I like MPD and felt like contributing. But I have more pleasant things to do. Please delete my repository from the musicpd site, Best regards. jf Max Kellermann writes: On 2013/12/18 10:02, j...@dockes.org wrote: +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. I do not think that this is useless bloat, except if you consider that std::string is useless bloat in general. As most of the code is based on std::string, defining the constant as such means that we do not incur a char*-to-std::string conversion each time the constant is used (to construct another string). When would this conversion happen? Use nullptr instead of 0. 0 is obscure. 0 is not obscure, as it was the standard up to really recently (NULL usage was discouraged by Stroustrup himself, you can easily google for this). It WAS the standard. MPD was not designed for this outdated standard. It was rewritten to C++11. When MPD was C99, it used NULL - Stroustrup's opinion does not matter here, because he was talking about C++. Now MPD uses C++11, and it shall use nullptr - Stroustrup's opinion still does not matter, because it's obsolete. I am all for converting to nullptr, but saying that 0 is obscure is a bit of an exaggeration. It obscures the nature of the variable. Using 0 implies that it's an integer, but it's not. It was obscure even when it was the standard. This old standard is obsolete for a good reason. I can change the declaration order if you really insist, but you should be aware that many people (e.g. Google standards) quite logically recommend to put the public declarations (the interface) first, and the private stuff (including the data members) last because this improves readability. There are arguments both for both sides. It boilds down to personal opinion. There are no logical super-arguments for any. I criticized your coding style because it does not follow what the rest of MPD does. Google standards are irrelevant here. The variables are not passed as std::string but as const refs to std::string, so, no overhead. Const or not const does not matter for the question of whether it's overhead, does it? It is overhead because a simple string literal must be converted to std::string each time the function is called - and that implies a heap allocation and deallocation. I actually don't understand your remark. Using std::string for everything except when constrained by an external interface is just good c++ practise. Maybe in your opinion. It's not in my opinion. MPD uses C++ syntax because it is easier to write than C. I converted MPD from C99 to C++ when C++11 came, which allowed me to eliminate most of the runtime overhead. Only then was C++ acceptable for me. Now you're adding all the C++ bloat that I feared, why I have been disliking C++ for so many years. Using a writable ref for returning the result avoids copying a possibly expensive object. This is also just good practise and I think that it's not clear to you because you are not used to it: non-const refs are for returning data, else they would be const... You may want to read https://en.wikipedia.org/wiki/Return_value_optimization What is reallyOpen()? The method name is obscure. I have to delay initialization of libupnp until after mpd daemonizes, else the lib does not work. I don't know if the fork does it or some descriptor manipulation. So it's a kludge to work around a MPD initialization order problem. This should be well-documented in a code comment so it can be eliminated once the MPD core has been improved. And you should write a ticket to request this improvement. + if (m_root) + return true; What does that check mean? As reallyOpen() has to be called for every visit call this is checking if we need to actually do something or we're already done. Setting m_root is the last thing the method does when really working. I could put the test at the call locations, but I find it cleaner this way. Again, there should be a code comment explaining this kludge. + FormatDebug(db_domain, UpnpDatabase::reallyOpen\n); Do we need those debug messages? What value do they add? Well, they're used for debugging actually. I could remove them, but having
Re: [Musicpd-dev-team] UPnP db plugin
On 2013/12/18 14:43, j...@dockes.org wrote: Please delete my repository from the musicpd site, Done. I've also deleted your Mantis account, and I'll disable your mailing list subscription. Please do not bother me again after calling me a liar. -- 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=84349831iu=/4140/ostg.clktrk ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] UPnP db plugin
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. jf -- 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=84349831iu=/4140/ostg.clktrk ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] UPnP db plugin
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, vectorstring 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 mapstring, TagType propToTag = {
+static mapunsigned 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=84349831iu=/4140/ostg.clktrk
