Max Kellermann writes:
> 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.
Ok, this is fixed (in the upnpdetails branch), I'll squash the commits when
we're done.
> Use pkg-config instead of AC_CHECK_LIB. That's more reliable and
> easier to use.
Done, I replaced with MPD_AUTO_PKG
> +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).
I imagine that, depending on the details of the compiler and lib
implementations, it would be conceivable that the conversions are optimized
away in all cases, but, by defining the variable as std::string, we are
sure of what happens and make things clearer.
> 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).
I am all for converting to nullptr, but saying that 0 is obscure is
a bit of an exaggeration.
> 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.
Using m_ prefixes for instance variables (not "variable names") makes it
clear inside the methods which is a local variable or parameter and
which is an instance one.
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.
> +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.
The variables are not passed as std::string but as const refs to
std::string, so, no overhead. I actually don't understand your
remark. Using std::string for everything except when constrained by an
external interface is just good c++ practise.
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...
> 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 the Open() method in the Database object does nothing and reallyOpen()
is called on the first actual usage of the object (e.g. Visit) and does the
actual initialization. reallyOpen() really opens... Maybe I should call it
maybeOpen() actually, this is what I usually do for this kind of
thing. What do you prefer ?
> + 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.
> + 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.
> + // 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.
> + // 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.
> +// 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.
> +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.
> + 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?
I have no objection to ifdef'ing out all debug code.
> +/* 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.
> + * 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.
> +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, the PTMutexInit/PTMutex pair is a
Resource-Acquisition-Is-Initialization wrapper for a pthread standard
mutex. This is extremely useful to make sure that mutexes are freed when
they need to be, making functions exception-safe and avoiding multiple
calls to free the mutex: the compiler takes care of doing it when the
critical section (materialized by a c++ scope) exits.
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.
> +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
> +#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.
> +#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.
sstream is part of libstdc++ which is a single shared object. So bloat ?
> + 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. This is a very
common idiom to protect these operators from being used by mistake from
external code. I know that c++11 has added other ways to do this, but using
expletives and insults about something just because you don't know about it
is quite impolite.
See for example this stack overflow question:
http://stackoverflow.com/questions/1008019/c-singleton-design-patter
I don't claim that my implementation is perfect (for exemple I think that I
would not need to provide implementation at all, I could just use
declarations), I can make mistakes, but really, calling this insane is a
bit beyond normal.
> +#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.
I agree.
> + 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 p in pthread stands for POSIX by the
way.
Cheers,
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=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
