Two style questions

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

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.

Scoped_ptr must be the simplest smart pointer existing, but useful:

// scoped_ptr mimics a built-in pointer except that it guarantees deletion
// of the object pointed to, either on destruction of the scoped_ptr or via
// an explicit reset().

Basically:

Template< class T >
Class scoped_ptr {
  T* ptr;
Public:
  Explicit scoped_ptr(T* p=NULL):ptr(p) {}
  ~scoped_ptr() { delete ptr; }

  // function get, op -> ,...
}

Attached the scoped_ptr extracted from boost (just modified the include
style from <> to ""), there is some accessory (but useful) features which
could be deleted as the complete_type delete checker.

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

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

Attached: patch1.patch

Cédric

patch1.patch (3.47 KB)

scoped_ptr.patch (23.8 KB)

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?

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

Attached: patch1.patch

Applied, thanks!
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20071112/002838.html

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.

Scoped_ptr must be the simplest smart pointer existing, but useful:

Sounds great.

Attached the scoped_ptr extracted from boost (just modified the include
style from <> to ""), there is some accessory (but useful) features which
could be deleted as the complete_type delete checker.

Please cut this down significantly more. We don't currently support old versions of metrowerks or sunpro, so those and the boost workaround stuff can all go away. Also, instead of BOOST_ASSERT, please just use <cassert> and assert. Please make it into a single header that we can check into include/llvm/ADT. Thanks!

-Chris

> Attached the scoped_ptr extracted from boost (just modified the
> include
> style from <> to ""), there is some accessory (but useful) features
> which
> could be deleted as the complete_type delete checker.

Please cut this down significantly more. We don't currently support
old versions of metrowerks or sunpro, so those and the boost
workaround stuff can all go away. Also, instead of BOOST_ASSERT,
please just use <cassert> and assert. Please make it into a single
header that we can check into include/llvm/ADT. Thanks!
-Chris=

Here a new patch. I didn't know what to do for the license header, so I let
it as is.
I also let the checked delete because pretty useful. The two free function
at the end: swap et get_pointer are only useful for generic programming, I
let then but don't know If it is really useful.
The other header in ADT seems to use Camel notation for their class name,
should I rename scoped_ptr => ScopedPtr. Also the indentation is 4 spaces
against the 2 of llvm, should this be corrected?

scoped_ptr.patch (3.1 KB)

Attached the scoped_ptr extracted from boost (just modified the
include
style from <> to ""), there is some accessory (but useful) features
which
could be deleted as the complete_type delete checker.

Please cut this down significantly more. We don't currently support
old versions of metrowerks or sunpro, so those and the boost
workaround stuff can all go away. Also, instead of BOOST_ASSERT,
please just use <cassert> and assert. Please make it into a single
header that we can check into include/llvm/ADT. Thanks!
-Chris=

Here a new patch. I didn't know what to do for the license header, so I let
it as is.
I also let the checked delete because pretty useful. The two free function
at the end: swap et get_pointer are only useful for generic programming, I
let then but don't know If it is really useful.

Sure, this looks great, very nice.

The other header in ADT seems to use Camel notation for their class name,
should I rename scoped_ptr => ScopedPtr. Also the indentation is 4 spaces
against the 2 of llvm, should this be corrected?

I don't have a problem with this.

Two requests:

First, please add the llvm copyright header and standard file prolog above the boost copyright blurb.

Second, please add an entry to the end of llvm/LICENSE.TXT with a pointer to the boost license.

Thanks Cédric!

-Chris