FreeLists / openbeosstorage / [openbeosstorage] some suggestions

[Ingo Weinhold <bonefish@xxxxxxxxxxxxxxx>]

Hello all,

being new to the team, I have quite recently scanned formerly produced 
documentations and code and tried to get familiar with the task.
As a side effect I now have some comments to the read documents and
some ideas/suggestions concerning the development process.

First the specific items:
* Statable.h: set_stat() is declared before the _OhSoStatable*() methods,
  but after them in the R5 header. This breaks binary compatibility for
  this class (not for the kit though).
* Path.h: MustNormalize() could be static.
* Entry.h: `#endif // __sk_path_h__'
* vfs docs, read/write/remove_vnode: As to be read in the ISO FS code
  (/boot/optional/sample-code/add-ons/iso9660) the `char r',
  parameter is a re-enter flag:
// fs_read_vnode - Using vnode id, read in vnode information into fs-specific 
struct, 
// and return it in node. the reenter flag tells you if this function
// is being called via some other fs routine, so that things like
// double-locking can be avoided.

This directly leads to some more general questions:
* What do you think about code reviewing?
  I know, it sounds boring (and probably mostly is), but it surely will
  increase the quality of the code. And sometimes one can learn something
  new (or something old used  in new or interesting manner).
  I'm not sure whether I'd like to have one guy, who reviews every code,
  or a sort of round robin system, so that this work is distributed
  evenly. In the first case usually the team leader does the job (or
  -- having the power to do that -- designates someone else ;-), but not
  to put more work on Tyler, I would volunteer to do it (of cource, I
  can't review my own code).

  The reviewing process would be the following: After an author claims a
  module to be finished, the reviewer thoroughly reads it. Any remarks,
  including bugs (i.e. incorrect or only partially correct code), bad
  programming style, documentation errors or just things that could not
  be understood, are sent to the author, who either agrees to the points
  and does the respective changes or discusses them with the reviewer. If
  no consensus could be found, the issues are taken to the mailing list.

* How to test binary compatibility?
  As soon as we can compile out classes with USE_OPENBEOS_NAMESPACE, we
  can write a test app, that compares the sizes of R5 and OpenBeOS class
  instances. Probably it is possible to test the vtables as well?

* Am I the only one, who thinks the R5 storage kit classes are strangely
  designed (to use moderate terms)?
  E.g. why the heck has each class all its directly derived classes as
  friends (see BStatable, BNode) instead of declaring certain members
  protected? It seems to be a way to prevent the application programmer
  from write subclasses that can access those members too.

  Personally I vote for reducing those dependencies where possible, i.e.
  to make stuff needed by subclasses protected and to analyze whether
  friendships to other classes are really needed.

  For implementing BFile, it was necessary to access BNode::fFd and
  BNode::fCStatus. Instead of doing it directly, I wrote accessor methods
  get_fd() and set_status() and since I suppose, they would be useful for 
  BDirectory and BSymLink too, I'd rather move them into BNode and make
  them protected (as well as set_fd(), close_fd() and others).

I'm sure I forgot something. Mmh...

CU, Ingo