[Gtkradiant] bug800 changes - my opinion

[Joseph, William]

>	A list of broken things with bug800 patches:

>	http://www.qeradiant.com/wikifaq/index.php?Code%20Conventions
>	name classes correctly, name headers correctly, put them in the right =
places

Archive code uses a consistent convention within each file...
for the new files added:
Global type names are 'TypeName'.
Member type names are 'Typename' or 'typename_t' or 'typename_type'.
Member variables are 'm_variable'.
Other variables and parameters are 'parameter' or 'variable'.

The important thing is to distinguish type names from other identifiers.
I avoid mixing more than one convention in the same file.



>	- ibytestream.h: remove this file completely. use IDataStream and =
extend idatastream.h header

InputStream/OutputStream/SeekableStream cannot be integrated with =
IDataStream in any meaningful way.
It would make sense to replace all uses of IDataStream with these =
instead.
This is not practical to do immediatly.


>	- vfspk3/archive.cpp: typedef DIR* dir_t; <- bloat, using DIR* is fine

The POSIX dirent interface is non-standard on win32 and is provided by =
glib-1.3 - which means a glib dependency.
Glib2 replaces dirent stuff with OS-independent GDir.
dir_t and the dir_* functions are an abstraction of POSIX dirent =
interface.
Using a clean abstraction now means we can swap DIR for GDir without =
changing the code that uses dir_t.
Fewer code changes means less work and fewer mistakes.
A clean abstraction with clearer naming also makes the code easier to =
read.


>	- WARNING: directory path does not end with separator
>	happens a lot during startup
>	GtkRadiant convention is to use trailing / on all directory paths

I added the printing of this warning every time the app gives vfs a =
non-standard path.
If you get this message, you can track down the problem and fix it at =
the source.


>	- class DirectoryListVisitor : public IArchive::IVisitor
>	don't declare classes inside classes. useless
>	specially in this case

IVisitor is not designed to be a generic interface - it is deliberately =
linked with IArchive interface.
This allows you to use the short form of the name in implementation of =
IArchive without worrying about name collisions.
Interface headers are included at a high level, so every effort should =
be made to contain names, either by using namespaces or nesting of =
types.


>	- IVisitor to it's own header

This makes no sense.. see above.


>	- class DirectoryListVisitor : public IArchive::IVisitor
>	declared and implemented at the same time in vfspk3/archive.cpp
>	use declaration, then implementation

Providing the implementation is short, simple and clearly written (it is =
in this case), I find it easier to write, read and modify the code if =
the declaration and implementation are in the same place (with =
implementation inlined).
With methods written inline, the compiler can choose whether to do the =
function call or inline the function (if it is not virtual).
Methods written inline make it efficient to create new layers of =
abstraction without function-call overhead.
Clean abstractions can help to reduce dependencies, making the code =
easier to maintain.


>	- DirectoryArchive::release: trash, use refcount class

The release method of IArchive and IArchiveFile is the equivalent of a =
virtual destructor, but allows the implementor more freedom.
The design of IArchive and IArchiveFile does not require =
reference-counting in an implementation.
The design allows for efficient and largely automatic resource =
management without reference counting.
Reference counting might make more sense on the _QERArchiveTable =
interface as part of synapse.