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 plugin starts up (when called from
> reallyOpen()), it broadcasts a discovery message asking all servers on the
> network to reveal themselves by answering.
>
> The way UPnP works, this initial message contains a time constant which
> defines the maximum for a random answer delay (this is to avoid that all
> servers would answer at the same time).
>
> This is often not understood by people implementing UPnP, and mishandling
> this means that you do not see all servers (in a random way as this depends
> on their random choice of delay), or that servers appear after a very long
> time (because they appear when they perform a spontaneous broadcast of
> their existence later on, the period for this might be a few minutes).
>
> Once I initialize the UPnP discoverer, I have to wait for the delay I
> specified (2S) to be sure that I'll get answers from all servers on the
> network. This is done only once on the first access (reallyOpen()). The
> list of servers is later maintained by listening to spontaneous broadcasts
> of servers: either periodic, or when appearing or going away.
>
> There is absolutely no going around this delay, but I agree that using a
> numeric constant here is inelegant, this should be part of the lower layer
> interface.
So you argued that this is ultimately necessary due to the definition
of the UPnP protocol. But blocking the MPD process is a bad idea.
Can't you integrate this in the event loop?
> > + // TBD decide what we do with the lib and superdir objects
> >
> > What does that mean?
>
> I don't know if I should destroy the listener and deinitialize the lib at
> this point.
Then please find out, and resubmit as soon as you found the solution.
If you don't, nobody else will.
> > +// 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.
>
> Ok, this is not API, it's an internal static function, but I can change it.
Being internal does not make this not an API.
> > +static map<string, TagType> propToTag = {
> > +static map<unsigned int, string> tagToProp = {
> >
> > Bloat! Use tag_table instead.
>
> I only use stdlib containers. I have decided this around 1995 while reading
> MySQL code (MySQL was entirely based on the STL, which was very bold at the
> time). stdlib containers are understood by every c++ programmer, bug-free
> and generally more efficient than local ones.
Facts check:
+static map<string, TagType> propToTag = {
+ for (auto& pt : propToTag) {
I benchmarked this with your code and tag_table. I ran this loop
100000000 times, adding the TagType integer values.
std::map 2414.535645 task-clock
tag_table 497.544526 task-clock
Then I benchmarked tagToProp:
std::map 387.933979 task-clock
tag_table 232.586354 task-clock
This time, it's not about personal taste. I could easily prove that
your assumption of "stdlib containers" being "more efficient" is wrong
in this special case - quite the contrary, tag_table performs much
better!
Besides being a CPU hog, the std::map bloats my benchmark program to
four time its size (9 kB instead of 2 kB).
> > +/* 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!
>
> Ok, this is quite funny, because this is actually the block that tells
> emacs to use tabs for indentation (indent-tabs-mode: t). The rest is for
> displaying, not saving.
Not true. This is how emacs estimates how many tabs to use to indent
a continuation line.
>
> > + * 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.
>
> It makes the actual code clearer. This is an advantage.
I don't see that, but that's personal taste.
> > +class PTMutexInit {
> >
> > MPD already has a Mutex class, one that is portable. Why do we need a
> > new one, one that is not portable?
>
> This is not a Mutex class
PTMutexInit is a Mutex class and nothing else. And it's not portable.
And it's bloated. MPD's pthread_mutex_t wrapper is constexpr, yours
requires runtime initialization!
> the PTMutexInit/PTMutex pair is a
> Resource-Acquisition-Is-Initialization wrapper for a pthread
> standard mutex.
You mean similar to MPD's ScopeLock class?
> libupnp uses pthreads internally. I have to synchronize with it, so it's no
> use pretending that this is going to work with something else. On platforms
> where pthreads are not available, you just can't use --enable-upnp.
pthread is part of libupnp's ABI, but not part of libupnp's API.
>
> > +extern std::string path_getfather(const std::string &s);
> >
> > MPD already has code for dealing with path names. No more code
> > duplication!
>
> It's bad style for a plugin to call into random interfaces inside the main
> program. Besides the "paths" here are locally and artificially generated
> and I don't want to depend on external behaviour
MPD's core API is not "external behaviour" nor is it a "random
interface".
> > +#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.
>
> These are the libupnp logging functions. These are all compiled out during
> a normal mpd build anyway.
But why use the libupnp logging library when this is MPD code?
If it's compiled out, what is the use of this, anyway?
> > + LibUPnP(const LibUPnP &) {}
> > + LibUPnP& operator=(const LibUPnP &) {return *this;};
> >
> > W T F. An assignment operator/constructor that does nothing. That is
> > insane code!
>
> Ok, I guess that you missed the "private:" specifier above.
No, I did not miss that.
> This is a very common idiom to protect these operators from being
> used by mistake from external code.
But it does not protect them from being used my mistake from internal
code. It is bad style.
The right way to do this in C++03 was using a "noncopyable" base
class, which did not have these disadvantages. The right way to do
this in C++11 is use "deleted" constructors/operators.
But there is no excuse for the way you did it.
> See for example this stack overflow question:
> http://stackoverflow.com/questions/1008019/c-singleton-design-patter
Not that Stackoverflow is a credible authority for "good style", but
even that code on Stackoverflow is different (better) than your code.
> > + if ((err = pthread_create(&thr, 0, workproc, arg))) {
> >
> > That code is not portable. Please use MPD's portable threading
> > library.
>
> No, and I explained why above.
The above did not convince me. I will not merge it that way until you
adjust that (and the rest).
> The p in pthread stands for POSIX by the way.
I know, but thanks anyway for the lesson.
------------------------------------------------------------------------------
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
