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

[David Abrahams]

A few more points:

. Coding conventions - please try to follow the coding conventions
established throughout the rest of the codebase.
class_base::get_context_object() is an example of a function which does all
kinds of things differently. IOW, not this:

    if(expr) {
        ...
    }

but this, please:

    if (expr)
    {
        ...
    }

Also, this function is much too big, dense, and deeply-nested. Comment
lines should in general be preceded by a blank line. Many lines are too
long. I wouldn't make such a big deal about this one, but I know I
mentioned it as early as
http://aspn.activestate.com/ASPN/Mail/Message/1230478.

. What is going on with transfer_attributes()? I don't understand why it's
there, yet. Why didn't you make it accept type_handle arguments instead of
object? That would have made the code far simpler...

. Variable names like "r" for a type_handle don't say enough about its
purpose to help the reader.

I'm still reformatting and massageing this get_context_object() thing so I
can get a feel for what it's doing...

-Dave

----- 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: [C++-sig] Questions/Comments about new stuff


> Hi Dave,
>
> Thanks for checking in the new stuff. It's wicked cool! There are some
> things I want to tweak, and some others that I have questions about.
>
> . What's the point of this module_info class? It looks like it should be
a
> namespace. Not everything should be an object...
>
> . 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...
>
> . get_prior_module really gets the current module, doesn't it? shouldn't
it
> be called module::current()?
>
> .  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.
>
> . 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.
>
> . 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 contains a comment which says it's called embed_test.cpp
>
> . 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.
>
> . 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.
>
> . 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.
>
> . 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.
>
> . 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.
>
> . 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 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 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_<>?
>
>
> . 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.
>
> . 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.
>
> . 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...
>
> -Dave
>
>
>
>
> _______________________________________________
> C++-sig mailing list
> C++-sig at python.org
> http://mail.python.org/mailman/listinfo/c++-sig