Re: [Firebird-devel] builds/win32/all.log & development roadmap

[Mike N. <ta...@al...>]

Olivier Mascia wrote:

> After  a  MSVC6  release  build  of  branch  1.5, builds/win32/all.log
> contains  some  messages
[...]
> Are these things  'tagged'  for  later (unscheduled) review and fix

That is the intention of those messages.

> or are these things  considered  to require urgent fix before any release

Probably not. Were they considered that urgent, they would probably have
been taken care of by now.

> Of course, I understand some of those messages just 'pin'  bad practises
> (code works anyway), but there are others looking more serious.

That's basically depending on who you're asking, and what you are asking
about. From a code correctness POV I'd believe most if not all of them
warnings are good. From a behaviour POV (i.e. running FB) it often makes no
difference whatsoever.

> Is  it  okay to take some of those, with no specific order and as time
> permit and submit here patches for review ?

Hell yes! These are long-standing slight problems waiting for fixups. The
reason I started adding them warnings was for just that reason - someone
else would grab the stick and do something about them.

> Or is there a more clearly defined  roadmap to follow on what to do
> first (or last) to positively contribute ?

Not really. We still carry a *lot* of "ugly" code from the past, and FB must
sooner rather than later get rid of that.

> Just a small excerpt for illustration :
> ...
> --------------------Configuration: engine - Win32
Release--------------------
> cvt.cpp
> C:\Dev\firebird\r15\src\jrd\../jrd/quad.cpp(37):Fix this! Ugly function
pointer cast!
> C:\Dev\firebird\r15\src\jrd\cvt.cpp(217):Fix this! Ugly function pointer
cast!
> cvt2.cpp
> C:\Dev\firebird\r15\src\jrd\cvt2.cpp(100):Fix this! Ugly function pointer
cast!

These three are mostly (from reading the post) not that urgent. However:

> dyn.cpp
> C:\Dev\firebird\r15\gen\jrd\dyn.cpp(944):TMN: FIXME! We do not have a
jmp_buf anymore!

Shit! Is that one still left?! As I wrote it with that certainty (TMN is
me) - it must be a real problem waiting to happen! Just from my wording I'd
say that code needs to be re-evaluated wrt. C++ exceptions.

> dyn_del.cpp
> C:\Dev\firebird\r15\gen\jrd\dyn_del.cpp(2551):TMN: LOCKSMITH!

I've been told that can be disregarded. Beats me why the same person didn't
remove the compile-time warning. Input, anyone? (I remember adding that
"warning" just because locksmith had been illuminated at the time - later
I've been told "this is not a security risk anymore" and if so, the warning
is today wrong).

> err.cpp
> C:\Dev\firebird\r15\src\jrd\err.cpp(514):FIXME! C functions can not throw!
FIXME!

Oh no, that one is still left? OK. That is *the* worst problem right now...
Last time I checked that code indeed was/could be a real problem. The way to
do it (least burden and most reward) I had in mind was a similar function,
called by this function, that catched C++ exceptions and returned error
code(s) accordingly.

Since I haven't looked at the code for cuite some time, it could be that
this function itself could catch C++ exceptions and return C status errors.
Either way, until that is 100% solved that comment stays - and displays a
problem.

> C:\Dev\firebird\r15\src\jrd\err.cpp(553):Fix! Can we change the API
> to take the real type?

Just a notification for coming developers. Nothing to worry about.

> event.cpp
> C:\Dev\firebird\r15\src\jrd\event.cpp(56):FIXFIXFIX!!! - DANGER!

If I wrote that (and from the wording I'd believe it was me) that code
*really* has to be examined and compared to what 1.x did. I havent checked,
why I don't even know what that code does, but from the wording it seem
pretty serious.

> evl.cpp
> C:\Dev\firebird\r15\src\jrd\evl.cpp(4161) : warning C4307: '+' : integral
> constant overflow

Technically the compiler is right (this is a compiler warning) and this
should be fixed. However, currently we're all using 2-compliment machines
and (unfortunately I'd say) this works as expected by that fact. It should
still however be fixed.

/Mike