[C++-sig] Re: Questions/Comments about new stuff

[Dave Hawkes]

----- Original Message -----
From: "David Abrahams" <david.abrahams at rcn.com>
To: "Dave Hawkes" <daveh at cadlink.com>
Cc: "pysig" <c++-sig at python.org>
Sent: Wednesday, July 17, 2002 8:53 AM
Subject: Questions/Comments about new stuff


> . What's the point of this module_info class? It looks like it should be a
> namespace. Not everything should be an object...
>
It holds the running information about the module being created, so that
objects can be added to the previously declared module. I'm not sure that a
namespace would be correct for this.

> . You have put get_module_info/set_module_info in the public interface of
> module_base, and thus in the public interface of module. Why? If it's
right
> (I hope it's not), then you need to move module_info out of the detail
> namespace...
>
I had some issues with it being private and building with gcc, though it was
OK with VC6 and I quickly moved it yesterday as a quick workaround.
Basically the issue for get_module_info is that it is used in the class
initialiser list for module_base and gcc complains. This can be worked
around using friends and then made private again.

set_module_info is another matter this needs to be accessed from with hte
module_init code I can make it private using either friends or an
intermediate class.

> . get_prior_module really gets the current module, doesn't it? shouldn't
it
> be called module::current()?
>
I was looking at it as the previously declared module as it is used in the
construction of current module declaration. But you could think in terms of
the current module and next module declaration.

> .  PRE_INIT_FUNC must be BOOST_PYTHON_PRE_INIT_FUNC, and it should be
> #undef'd when you're done with it. We also need to get our generated
> init_module_#xxx stuff into the unnamed namespace.
>
I can tidy that up fairly easily.

> . What is "get_class_context_object()"? I'm not fond of that name. I think
> in Python that's called a name space. I would prefer renaming that
function
> "name_space()". If the handle<> can never be NULL, it should return object
> instead.
>
It get the nested class context in which to add the current class
information. I didn't call it a namespace as that may imply its a generic
namespace where in fact it only applies to class definitions. It can legally
return NULL so I had to use handle() for the return.

> . submod_subclass_api.cpp should be renamed, since a subclass is not the
> same as a nested class ;-), and putting "api" in the name of a library
test
> is sorta redundant. How about "nested.cpp"?
>
It's not complete yet as it will also test the api module when it's
complete. Also it will eventually accept an argument that will select the
test to perform so it can installed as 3 separate tests. In fact it could be
extendible to additional suites of tests at which point the name in fact is
inapropriate.
> . It contains a comment which says it's called embed_test.cpp
>
Missed that in the clean up...
> . It contains lots of macro definitions which are never used. These should
> all be stripped out, as they harm readability. In this particular case,
the
> macros that do get used don't save any more text than the definition of
the
> macro cost, so they should be removed also.
>
This was boiler plate that came from something larger. I was planning on
adding more tests so the macros would be more useful but that hasn't happend
yet. I can certainly remove them for now.

> . I'm not happy with the name 'call_statement'. After all, you can pass it
> more than one statement, and you're not calling anything. I'm not sure I
> have a better name for it yet, but what about eval()? I realize Python has
> a different eval(), but I think it's got equivalent applications.
>
eval is already implemented to emulate what eval does. Remember eval returns
the result of an expression. In fact its closer to exec in functionality,
but that's already defined as well...
I couldn't think of a better name either and unfortunately its stuck with
the first one that came in to my head.

> . call_statement_du should be renamed to simply "call_statement".
> Overloading should work; if it doesn't we have a problem. In any case,
> abbrevs sch as du shld be avoid.
>
It was call_statement until yesterday when I though I was having some issues
with gcc 3.1, so I changed it just in case until I could do some more
testing. This is meant to be a private interface so the user should not use
it.

> . You kept get_arg<>, despite my challenges... OK, but you didn't respond
> to my questions about it either. I hope you understand, I have to either
be
> satisfied that it's the right choice, or rip it out. I guess the point is
> that you're saving an Py_INCREF()/Py_DECREF() pair for each object passed?
> I'm still unconvinced it's worth it. Have you done any measurements? Note
> that the way you've written it, it won't work for classes such as list
> which are derived from object. I also don't understand why there's an
> object const* converter. I'm still very uncomfortable with this.
>
I was trying to save the additional object constructution/destruction. If
you feel it's not worth it I can certainly take it out. Yes it would need to
be adjusted for the addional object derivatives. So maybe its not worth it
from a maintenace point of view. I'll take it out for now.

> . py_interface.hpp might be too big, IMO. People like to be able to select
> how much stuff gets dragged into each translation unit by using
> high-granularity headers.
>
As I started adding to it I did wonder about this myself. Currently tt's not
excessively large and it doesn't need much preprocessing so it should be
fast to compile. However it's probably worth thinking about some API groups
that it may be split along later. I was thinking about having a header that
included all the API headers for those that want everything and then the
individual headers with API groups for those that wish to fine tune.

> . It's still using get_func("...") in function templates, which will
> generate too much code in extension modules. For example,
>
> BOOST_PYTHON_DECL object slice(object const& a0, object const& a1);
> BOOST_PYTHON_DECL object slice(int a0, int a1);
> template<class A0, class A1, class A2>
> object slice(A0 const& a0, A1 const& a1, A2 const& a2)
> {
>     return api_detail::get_func("slice")(api_detail::get_arg<A0>(a0),
> api_detail::get_arg<A1>(a1), api_detail::get_arg<A2>(a2));
> }
>
>
> That should be rewritten:
>
> BOOST_PYTHON_DECL object slice(object const& a0, object const& a1);
> BOOST_PYTHON_DECL object slice(int a0, int a1);
> template<class A0, class A1, class A2>
> object slice(A0 const& a0, A1 const& a1, A2 const& a2)
> {
>     return slice(api_detail::get_arg<A0>(a0).operator object const&()
>         , api_detail::get_arg<A1>(a1).operator object const&()
>         , api_detail::get_arg<A2>(a2).operator object const&());
> }
>
I'll get rid of the get_arg and rework these anyway.

> . I don't like this change:
>
> -    class_();
> +     class_(base const& parent_class = empty_class_base());
> -    class_(char const* name);
> +    class_(char const* name, base const& parent_class =
> empty_class_base());
>
> a. Default arguments generally create more code than using overloads
> b. A class with no parent class doesn't have an empty class as its parent
> c. Parent is the wrong name -- should be "enclosing"
>
I can change these to get rid of the defaults. I'ts not likely the same
template pattern would be used with a different parent_class argument so I
suppose 2 different inline constructors should not be instantiated. Maybe
I'll pull some common code out of the constructors anyway as we are going to
have 4 versions.

> . I don't like this change:
>
> +    // add to module
> +    self& add(module &m)
> +    {
> +        // redundant
> +        // m.add(*this);
> +        return *this;
> +    }
> +
>
> Calling a.add(b) looks like it adds b to a, not the other way around.
> Should these add() functions really appear in the public interface of
> class_<>?
>
>
The act of instantiating a new module automatically adds itself (in the
constructor). I left this in accidently as it was there to support my own
old code! I'll remove it in the next batch of fixes.

> . Likewise for the rest of the changes to class.hpp
>
> . This change really worries me:
>
>      module& add(class_<T1,T2,T3,T4> const& c)
>      {
> -        this->add_class(c.object());
> +        // redundant
> +        // this->add_class(c.object());
>          return *this;
>      }
>
> You can't just comment stuff out and expect it to work... Perhaps you're
> just leaving this function in so that existing code will keep compiling in
> the short term? I could understand that.
>
Classes are automatically added (in the constructor) to the last module that
was declared, therefore this code is left in so as not to break existing
code (which could have only one module anyway).

> . Your code generators use large swaths of C++ code which gets stuffed
into
> source and header files, e.g. DeclFileHeader. Couldn't these just be
> written as C++ files which get #included in the result? Better yet, have
an
> outer file which #includes the generated and non-generated components.
>
Yes I know, initially there was only a line or two and then it grew...
I'll be removing the get_arg stuff which should cut it back a little.

> . What is your naming convention for the Python code? You have an odd
> combination of MixedCase and all_lower_case. There is a Python style guide
> somewhere...
>
I plan to tidy it up now it's basically functional. The origins of some of
the code were borrowed from something else I was working on, hence the mixed
style. I'm going to get rid of the capitalized items shortly.

Dave Hawkes