lucy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marvin Humphrey <mar...@rectangular.com>
Subject [lucy-dev] Making implementing functions static
Date Sun, 22 Apr 2012 22:12:42 GMT
Moving this to the dev list...

On Wed, Apr 18, 2012 at 10:34 AM, Nick Wellnhofer (Commented) (JIRA)
<jira@apache.org> wrote:
>
>    [ https://issues.apache.org/jira/browse/LUCY-231?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13256730#comment-13256730
]

> I wasn't yet thinking of making the implementing functions static, but to hide
> them from compiled extensions by not exporting them in the DLL.

Gotcha.

I can see an argument for making implementing functions visible up to the edge
of a parcel/DSO yet not public and accessible to extensions.  However, we have
such intra-parcel visibility now, and in my opinion it creates more problems
than it solves.

For instance, in the days before SUPER_METHOD, super-method invocations were
achieved by calling a manually selected function directly.

Consider the following object hierarchy.

    class Foo {
        // ...
        void
        Do_Stuff(Foo *self);
    }

    class Bar inherits Foo {
        // ...
        // Does *not* implement Do_Stuff().
    }

    class Baz inherits Bar {
        // ...
        void
        Do_Stuff(Baz *self);
    }

The implementation of Baz#Do_Stuff used to look like this:

    void
    Baz_do_stuff(Baz *self) {
        Foo_do_stuff((Foo*)self);  // <-------------- SUPER?
        // more stuff...
    }

Unfortunately, this practice left us vulnerable to an action-at-a-distance bug
should Bar implement Do_Stuff(). Suddenly, the hard-coded SUPER invocation in
Baz_do_stuff becomes wrong -- it should now invoke Bar_do_stuff() rather than
Foo_do_stuff() -- and this error occurs without anyone touching either Foo
or Baz!

We still have this problem for constructors, since Clownfish does not provide
any support for retrieving the superclass's init() function programmatically.
You actually patched a bug a little while ago where the wrong super init() was
being invoked:

    http://svn.apache.org/viewvc/lucy/trunk/core/Lucy/Object/Num.c?r1=1305401&r2=1310738

    r1310738 | nwellnhof | 2012-04-07 04:18:28 -0700 (Sat, 07 Apr
2012) | 3 lines

    Fix Int64_init

    Calling FloatNum_init was actually harmless, but it should call IntNum_init.

Additionally, as noted earlier, it is too easy to misspell a method and use
the lower-case variant directly, a "mistake" which multiple Lucy PMC members
have made when starting out.  It's not their fault -- they look at the C code,
they see the implementing function, they use it without realizing that they're
supposed to be using the autogenerated method invocation wrapper.  Or, it's
easy to just miss the <shift> key inadvertently.  I've done that numerous
times myself, and some of those bugs are still in Lucy.

Here's a report of the current places in core where an implementing function
is invoked directly:

    https://issues.apache.org/jira/secure/attachment/12523721/bad_encapsulation_report.txt

None of those are necessary.  Some are capitalization typos, some can be fixed
by using either SUPER_METHOD or METHOD.  If we address all of those, then the
only issue remaining is that the vtables are assembled in parcel.c rather than
the individual .c files.

For the record, if we make implementing functions static, it will still be
possible to publish a public wrapper if need be, and any decent optimizing
compiler will inline away the extra call:

    static void
    S_Foo_Do_Stuff(Foo *self) {
        // stuff...
    }

    void
    ext_Foo_do_stuff(Foo *self) {
        S_Foo_Do_Stuff(self);
    }

Furthermore, the implementing functions are still accessible via their
VTables:

    // These are equivalent:
    Ext_Foo_Do_Stuff_t a = CFISH_METHOD(EXT_FOO, Ext_Foo_Do_Stuff);
    Ext_Foo_Do_Stuff_t b = ext_Foo_do_stuff;

So, I don't believe we are taking away much by correcting this design flaw.

Marvin Humphrey

Mime
View raw message