announcement: Kwave is in kdereview

[David Faure]

On mardi 11 octobre 2016 21:41:09 PDT Thomas Eschenbacher wrote:
> David Faure wrote:
> > [...]
> > As a user, I definitely vote for kwave going to kdemultimedia.
> > As a developer that means I have to review it ;-)
> 
> yes, I vote for kdemultimedia too. Forget about extragear...

I meant, as a user I vote for kwave passing review and finally becoming part of 
the KDE applications using the KDE infrastructure :)
 
> > * OPTION(DEBUG "build debug code [default=off]" OFF)
> > Why not use the more standard CMAKE_BUILD_TYPE? You have many combinations
> > otherwise, some of them quite wrong.
> > CMAKE_BUILD_TYPE=Debug and DEBUG=OFF would lead to -DNO_DEBUG
> > -DQT_NO_DEBUG, quite unexpected.
> 
> That's right, but I remember that I had some trouble with that, AFAIR
> Q_ASSERT did not work even when setting the CMAKE_BUILD_TYPE. 

I can assure you that Q_ASSERT works when CMAKE_BUILD_TYPE is set
to Debug. It's my every day setup.

> Needs some
> more investigation, I have put this onto the TODO list. Currently it
> also switches on the debug plugin in the menu.

You can do that with a check on CMAKE_BUILD_TYPE.

> > main.cpp shouldn't use I18N_NOOP anymore in KF5. You should create the
> > QCoreApplication instance first, then create the KAboutData using i18n(),
> > then create the QCommandLineParser, also using i18n()  (or _ as you name
> > it).
> ^^^^^^^^^^^^^^^^^^^
> no, that's wrong. the _(...) macro has nothing to do with i18n

OK, so forget the last parenthesis, but everything else in the above paragraph 
stlil stands, please change the initialization order in main.cpp.
 
> > No NOOP needed anywhere anymore.
> > This means passing the parser to the app later than as a ctor argument
> > though, obviously.
> 
> No, I18N_NOOP is still necessary, can not be omitted.

And I can guarantee you, it can, and it must. Shall we keep playing the "I 
know better than you" rule or are we going to have actual technical arguments?
Here are mine: I wrote the KF5-based KAboutData, I wrote QCommandLineParser, 
and 100% of the KF5-based apps out there create the app first, as recommended 
by the Qt developers.
I'm listening to your actual counter-arguments (not "I remember vague issues 
from some time ago" ;-))

> I remember that I
> had spurious application crashes on startup that were caused by failed
> translations 

More details please. And was that really when creating the app first?

> and additionally at some points I must handle untranslated
> strings internally but have the possibility to translate them on demand
> later, thus I need an entry in the translation database.

This simply says "I need I18N_NOOP because I need to mark things for later 
translations", which is a tautology. You might need that somewhere, but not 
for KAboutData and not for QCommandLineParser, trust me.
 
> > Kwave::TopWidget::executeCommand: is the "if (false) {" around a lot of
> > code intended? Looks weird.
> 
> Yes, that's because a switch/case cannot be made on strings, so I wrote
> a macro to make the "if (string==...) else ..." cascades look a bit more
> readable (the "CASE_COMMAND" macro). I could write a macro to
> encapsulate/hide this, maybe some SWITCH_COMMAND() macro, but I'm not
> sure if it really makes things any better... Do you have a better idea?

I don't follow. How does the use of that macro mean that you need if (false) 
in your code? This code is dead, right?
Dead code is bad. It confuses the reader, it confuses translation extractors, 
etc.

I don't have a good answer to how to write a switch/case on strings, except 
from the obvious one: using an enum instead of a string, in the first place ;-)

> > Kwave::SwapFile should be ported from int to qint64 for all file sizes,
> > no?
> > (to go above the 4GB limit). All the INT_MAX stuff and the casting from
> > qint64 to int should IMHO be removed; even if 4GB is a crazy size, it
> > would still make the code nicer (no casts).
> 
> No. The point is that there are still some 32bit systems around. 

So? Even on 32 bit systems the QFile and QFileInfo APIs uses qint64.

> The limitation is not so obvious, but at some place these sizes/numbers go
> into GUI elements - and on 32bit systems these still are simple plain
> "int" values, thus 32 bit. 

If these values come from QFile or QFileInfo, they should be qint64.
On all systems.

> Additionally the 4GB is still a limitation
> for many file formats which cannot handle such huge files, so breaking
> the 2GB or 4GB limit is currently not something really urgent, nobody
> complained about that the past few years.

OK, valid point, but I would still make the code more robust and cast-clean.

> > Kwave::Stripe::resize detaches without the mutex locked, while other
> > methods in the same file do it with the mutex locked.
> 
> OK, that one needs some more time to check, that's not as trivial as it
> might look now. AFAIR it had have side effects / deadlocks in this area
> and changed the locked ranges for good reason.

That doesn't make me very confident about the code in question...
 
> > Is signal() really threadsafe? Kwave::WorkerThread::run surprises me with
> > its use of unix signals. Even if it works (which I didn't know), the
> > mutex here does not prevent multiple threads from calling it at the same
> > time, since the mutex is a *member* of the thread. Two threads, two
> > mutexes, no mutual exclusion. On top of that, "forbid sending SIGHUP"
> > will forbid sending SIGHUP to any thread, not just the one currently
> > exiting, right?
> 
> POSIX says that this signalling does not (need to) work in multithreaded
> environments. But in Linux it does. The signal is only sent to the right
> thread, so no problem here. That worked 100% stable for years.

OK, but surely signal(SIGHUP, old_handler) affects all threads, doesn't it?
How could this possibly affect just one thread?
If it does affect all threads, then you have a clear race there (described 
above).  The code is "stable" because the killing with a signal is probably 
only rarely used, I suppose. It's still buggy ;)

> > Still in that file, cancel() and shouldStop() should lock m_lock (bool
> > isn't an atomic type, contrary to what many people think, see C++11
> > memory model).
>
> That's not necessary, even if bool is not handled atomically, the
> evaluation against zero or non-zero still works, so which kind of
> intermediate states or race conditions could happen here? 

A data race is undefined behavior. This is my area of expertise, but if you 
don't trust me, I encourage you to read the C++11 memory model
or the book "Concurrency In Action".

> Would you feel better if I change it to a QAtomicInt or so?

Yes.

Not just me, but your compiler and your CPU will feel better too,
at moving from the area of undefined behavior to valid C++ :-)

> > Indentation: you seem to use 4 spaces for level-1 indentation, and then
> > one
> > tab for level-2 indentation. Vieweing the file in vim (which here seems to
> > use a tab width of 4) makes the file unreadable. Might I suggest
> > something more portable, like 4 spaces, 8 spaces? (Qt5/KF5 coding style).
> 
> That's just the style it always had. Kdevelop supports it. I also did
> not like that very much, but one gets used to it. I think changing it
> only produces a lot of traffic in the version control and no customer
> has any benefit from that.

It forces the occasional contributor to set up his text editor otherwise it's 
just unreadable. But if you don't want external contributions, ignore me :-)

More seriously, there is no KDE-wide coding style, although the KF5 coding 
style (which is very similar to the Qt coding style) is becoming more and more 
widespread. So I can't force you to do anything about this, but I still don't 
think that such a requirement on tab widths is wise.

> > Did you ever try compiling with clang? If not I recommend giving it a try,
> > it will point out many more things than my small brain can figure out ;-)
> Yes, I did and do that regularly. It has pointed out many things, but
> also had some bad side effects, like some plugin that stopped working
> due to strange linker breakage, some instrumentations did not work and
> AFAIR the combination with valgrind also made trouble...

This sounds like something worth investigating and fixing.
I know some people who compile everything with clang...

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5