[haiku-commits] Re: haiku: hrev48711 - src/servers/app/drawing
- From: Adrien Destugues <pulkomandy@xxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Fri, 23 Jan 2015 17:01:01 +0100
On Fri, Jan 23, 2015 at 03:24:23PM +0100, Stephan Aßmus wrote:
> Hi,
>
> Am 23.01.2015 um 13:46 schrieb pulkomandy@xxxxxxxxxxxxx:
> >hrev48711 adds 1 changeset to branch 'master'
> >old head: 84ed7b4bb4ad1c0f002ecc475e6f048904ffea58
> >new head: 6f3acb91631d60911121f71ebd92b15d5f08b63b
> >overview:
> >http://cgit.haiku-os.org/haiku/log/?qt=range&q=6f3acb91631d+%5E84ed7b4bb4ad
> >
> >----------------------------------------------------------------------------
> >
> >6f3acb91631d: app_server: fix misuse of BReferenceable.
>
> Can you please 1) use an appropriate description and 2) make sure your
> commits are actually correct? You are degrading the code quality with such
> hasty "fixes". 1) There was no "misuse" of BReferenceable. Just because the
> debug code of BReferenceable does not handle some perfectly valid use cases
> does not mean these perfectly valid use-cases are "misuse". 2) You've
> introduced a memory leak, since you have not adjusted the fBitmap
> initialization. You need to tell the reference that you already have a
> reference (the initial reference count of newly created references is 1). If
> you don't do that, the BReference acquires an *additional* reference and the
> bitmap is now leaked.
Sorry, but this was preventing use of BReferenceable in DEBUG=1 mode as
app_server would enter debugger as soon as I tried to open a window. I'm
trying to debug something else entirely and it is a bit annotying to hit
several similar problems with BReferenceable, some (including this one)
having bugs open about them for years, with no one attempting a fix.
What to do then? Wait until the original developer(s) have a look and
fix it? Or let the code this way, preventing anyone to use the debugger
features available in libbe?
Maybe it is not "misuse", since BReferenceable has no documentation and
no unit tests, this is open to interpretation. At least the debug code
there assumes that BReferenceable that are not allocated on the stack or
properly reference-counted are not allowed. I'm fine with allowing
BReferenceable objects to be allocated outside of reference-counted
contexts, and actually the original implementation allowed it (and after
fixing these 2 known "bugs", which I thought would be the only ones, I
can now get similar problems in several other parts of the code). But since
that was not documented, the implementation was then changed, and now it
is harder to use a BReferenceable outside of a reference counted
context, for example as a member of another class. While investigating
these problems I found a few other issues, for example the fact that
FirstReferenceAcquired will never be called because BReferenceable
initializes its reference count to 1, not 0. Looking at the git log,
this was also introduced because of misunderstanding on the API of the
class between the various developers who worked on it over the years.
I spent the most part of 2 days reviewing the BReference code and places
using it, to understand how it is designed. I have more (yet
uncommitted) changes to implement a reference to a const object. I'm
doing this because I want to use reference counting in some network
code. I expected I would only need to fix the decorator bug, and then I
also hit this one. This is dragging me further away from what I initally
wanted to work on, and I did not want to spend much more time on it.
>
> So instead of fixing anything, you actually broke it. Just saying. :-(
>
> Note that this regression is elliminated by your later commit which skips
> using the reference.
This shows that our code review process works, at least. However, I
agree that reviews would better be done before commits are pushed to
trunk, as this would avoid most of our build breakage and regressions
(ok, most caused by myself, but I spend a lot of time writing code
lately). For this reason I suggested that we setup a proper code review
tool. I first made this suggestion in the post-mortem mail I sent right
after the alpha 3 release, that was more than 3 years ago. We still
don't have such a tool, so I'm relying on the existing and suboptimal
code review process. Again, what else can I do? Stop writing any code
and wait for someone else to do it?
--
Adrien.
Other related posts:
