[Gtkradiant] bug800 changes - my opinion

[Timothee Besset]

On Mon, 12 May 2003 17:47:37 +0100
"Joseph, William" <WJoseph@europe.ea.com> wrote:

> >	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.
> 

I don't care about syntaxic remnants from QE4. We try to clean them as we
go. The official syntax for class names is in the wiki and hasn't changed
since 1.2.

Your code contributes new lines to the code conventions everyday :-)
http://www.qeradiant.com/wikifaq/index.php?Code%20Conventions

> 
> 
> >	- 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.
> 

It can. Should never have two independant APIs that do the same thing. See CHANGES on bug800 branch.

> 
> >	- 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.
> 

Ok, that's the correct way to go on that one.

> 
> >	- 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.
> 

Ok

> 
> >	- 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.
> 

If you think our current ref counting architecture doesn't allow us to do
correct resource management, then that's the current ref counting that
needs to be refined, and not something else written in parallel again. And
it would not be the first time I see someone saying that it can't be done
with the current API whereas it can (I remember discussions around synapse
and HL support in that regard).

See CHANGES file for a list of things that still need to be reworked for
IArchive. Those are based on the comments you made here.

TTimo

> 
> 
> _______________________________________________
> Gtkradiant mailing list
> Gtkradiant@zerowing.idsoftware.com
> http://zerowing.idsoftware.com/mailman/listinfo/gtkradiant
>