[cfe-dev] Two style questions

[Chris Lattner]

On Nov 10, 2007, at 11:42 AM, Cédric Venet wrote:
> First, clang didn't seems to use any smart_ptr. Is it by design?
> I would like to use a scoped_ptr (for class member or local  
> variable), what
> should I do:
> - do the freeing manually in the destructor or at the end of the  
> function
> (error prone and not maintenable)
> - use std::auto_ptr
> - use an home made scoped_ptr
> - use boost::scoped_ptr (I know you don't want to create dependency on
> boost, for simplicity of compilation, but we could extract the few  
> class
> needed)

I'm not familiar with boost scoped_ptr, and won't have time to dig  
into it for a couple of days.  Assuming it is simple and useful (i.e.  
it really does simplify the C++ code we end up writing and  
maintaining), I don't have a problem with adopting it.

Unrelated, in general, I don't like using refcounted pointers when  
they aren't needed though, simply to avoid the overhead.  IMO it's  
much better to design a clear ownership/memory model instead.

> Second, in a few place, typedef of void are used as opaque type (for
> reducing dependency or allowing various concrete type depending on the
> implementation) and I was wondering why not use empty struct instead:
>
> typedef void ExprTy; => struct ExprTy {};
>
> I can see than a void* is easier to cast (static_cast instead of
> reinterpret_cast) but the concrete type could derive from the empty  
> struct
> achieving the same without cost (thanks to empty base class  
> optimization).
> This would allow stronger type checking and less bug, or is there  
> any reason
> I didn't see?

That is possible.  It's an interesting idea that I haven't  
considered.  The one problem with it is that it forces a requirement  
on the implementations of (for example) the AST.

For example, if ExprTy became a distinct type from StmtTy, then our  
current AST would have problems (where Expr derives from Stmt).  It  
could be solved with multiple inheritance, but this just makes the  
class hierarchy more confusing.

I agree that lack of static type checking is badness though.  If you  
are interested in this, please put together a patch.  If it is clean  
and doesn't have bad consequences, then I'd be glad to see it go in.

> In particular, I don't see why BuilderTy in ModuleBuilder.h is  
> declared as
> an opaque type since it represente a CodeGenModule and could simply  
> use a
> forward declared class no?
>
> typedef void BuilderTy;
>
> =>
>
> Class CodeGenModule;
> typedef CodeGenModule BuilderTy;

You're right, this is a clear "bug", please send in a patch!

Thanks a lot for the attention to detail, it's always great to have  
another set of eyes on the code,

-Chris