Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forward declarations limitation in ICE 3.7 #97

Closed
Lastique opened this issue Jun 7, 2018 · 11 comments
Closed

Forward declarations limitation in ICE 3.7 #97

Lastique opened this issue Jun 7, 2018 · 11 comments
Assignees
Milestone

Comments

@Lastique
Copy link

Lastique commented Jun 7, 2018

In ICE 3.7 there is a breaking change with regard to forward declarations of interfaces and classes in Slice files. In 3.6, it was possible to forward-declare an interface or class and use its proxy in method parameters, return types, structures and classes. This is no longer possible in 3.7 as the compiler requires the complete definition of the forward-declared interface or class.

This is a severe limitation and a deal breaker in our case. We have used this feature extensively to reduce dependencies between different components, reduce compile times and memory consumption during compilation. Consider a use case with a factory interface that contains methods for creating objects of different types (i.e. having different interfaces). Previously, those interfaces were forward-declared, which allowed users of this factory to only depend on the interfaces they needed. Now, the users have to include all interfaces, including their dependencies, which makes the API pretty much monolithic.

This issue asks to revert the limitation or provide a suitable alternative.

@bernardnormier
Copy link
Member

bernardnormier commented Jun 13, 2018

As of Ice 3.7, most Slice forward declarations must be fully defined in the translation unit as described in the release notes.

Previously, you could forward declare and never define your interface or class, even if you used it! In C++, you would get a link error when linking your program or shared library if you forgot to provide (and slice-compile) the definition of this forward declared interface or class.

While the previous behavior was not documented, we expected that some applications would rely on this behavior and we made this breaking change only because we didn't find a better alternative.

With the C++98 mapping, proxies are mapped to our own C++ types, and we could craft them to support this forward-with-no-local-definition Slice feature. However, with the C++11 mapping, proxies are mapped to plain std::shared_ptr, and there is no easy way to support forward-declared only Slice interfaces or classes when the same translation unit also marshals or unmarshals the proxies or class instances.

It's also likely new language mappings (such as the new Java mapping or the new MATLAB mapping) rely on this new rule for Slice forward declarations.

@Lastique
Copy link
Author

I'm aware that the change is mentioned in the 3.7 release notes. I've also discovered that at least the modern Java mapping fails to be generated if the check for complete definitions is removed.

My point is that this limitation is too detrimental, at least in the C++ world, for the reasons I described in the initial post. I'm not familiar with the C++11 mapping yet, but it would seem not that different with regard to forward-declared proxies and classes because you can declare and use shared_ptr to incomplete types just as you could do it with IceUtil::Handle and its cousins. You cannot dereference the pointers, but that should not be required in the generated headers. You don't implement marshalling code in the headers, it is only implemented in the generated source files.

One possible solution that I see is to generate forward declaration headers along with the main headers. An #include directive in a Slice file would then be converted to an #include of the corresponding forward header in the generated main header and an #include of the corresponding full header in the generated .cpp file. For example:

a.ice:
// some declarations

b.ice:
#include <a.ice>
// some declarations

would translate to:

a_fwd.h:
// forward declarations from a.ice

a.h:
#include "a_fwd.h"
// complete definitions from a.ice

a.cpp:
#include "a.h"
// implementation for types from a.ice

b_fwd.h:
// forward declarations from b.ice

b.h:
#include "a_fwd.h"
#include "b_fwd.h"
// complete definitions from b.ice

b.cpp:
#include "a.h"
#include "b.h"
// implementation for types from b.ice

@bernardnormier
Copy link
Member

The marshaling and unmarshaling in C++11 is actually implemented in part in the header files, which results in less code bloat (only the code for the mapped function you use gets compiled). This also allows us to use template member functions for the future-returning async member functions.

But this is optimization is not the main reason behind this change. Take this Slice definition:

// in widget.ice
module Demo
{
   interface Widget;
   interface WidgetFactory
   {
      Widget* createWidget();
   }
}

slice2cpp generates widget.h and widget.cpp from this definition. Say we don't use any inline code in header files and all the marshaling code is in widget.cpp. How is widget.cpp going to marshal/unmarshal shared_ptr without seeing the full definition for Widget?

Note that you can still forward declare all you want interfaces that you don't use in your translation unit. For example, the following is completely valid in 3.7:

// file widgetF.ice
module Demo
{
    interface Widget;
    interface WidgetFactory;
}
// file widget.ice
#include <widgetF.ice>

module Demo
{
    interface WidgetFactory
    {
      Widget* create();
    }
  
   interface Widget
   {
      string getName();        
   }
}

Maybe the work-around for your application is to add Slice files with only forward declarations - like widgetF.ice above? And then include widgetF.h in C++ code that doesn't need the full declarations.

@Lastique
Copy link
Author

Lastique commented Jun 13, 2018

How is widget.cpp going to marshal/unmarshal shared_ptr without seeing the full definition for Widget?

With my suggested solution with the forwarding headers, we would not need to use forward declarations in Slice files as much because the dependency bloat would still be mitigated in the C++ generated code.

Other solutions I can think of:

  • Use separate Slice directives to include definitions of the types and interfaces (e.g. #include_impl). Those includes would only be added to the generated .cpp files.
  • Forward-declare marshalling routines as well in the generated headers so that the calls to those routines are resolved at linking stage.
  • Use a runtime lookup of the marshalling routines in a global table.

But frankly, the solution with the forwarding headers I like the most so far as it involves least overhead and does not affect the Slice language.

Maybe the work-around for your application is to add Slice files with only forward declarations - like widgetF.ice above?

No, that doesn't solve the problem. Basically, what we have now is:

module Demo
{
    interface Widget1;
    ...
    interface WidgetN;

    interface WidgetFactory
    {
        Widget1* create1();
        // ...
        WidgetN* createN();
    }
}

In 3.7, this means that whoever wants to use WidgetFactory has to include all WidgetN definitions and whatever definitions are mentioned in those interfaces, recursively. This practically means monolithic API, which could as well be fully defined in a single header. This is very costly, especially if N is large and the user only needs a couple of Widget types.

@bernardnormier
Copy link
Member

bernardnormier commented Jun 13, 2018

How would your suggested "_fwd.h" solution apply to your sample Slice file?

module Demo
{
    interface Widget1;
    ...
    interface WidgetN;

    interface WidgetFactory
    {
        Widget1* create1();
        // ...
        WidgetN* createN();
    }
}

Unless the generated code uses some C++ trickery (like we did in Ice C++ < 3.7), the generated .cpp file needs to see the full definition of the various Widget interfaces.

Now, if this Slice file also includes the definitions for all the Widget1...N interfaces, how would slice2cpp know you want the full WidgetFactoryPrx declaration in widget_fwd.h, but only a forward declaration for the other proxy types?

@Lastique
Copy link
Author

As I said, I would not need to use forward declarations in the Slice files and could include the Slice files with full definitions instead.

// File widgetfactory.ice
#include "widget1.ice"
...
#include "widgetN.ice"

module Demo
{
    interface WidgetFactory
    {
        Widget1* create1();
        // ...
        WidgetN* createN();
    }
}

The generated C++ files would look like this:

// File widgetfactory_fwd.h
namespace Demo {
class WidgetFactory;
class WidgetFactoryPrx;
}

// File widgetfactory.h
#include "widget1_fwd.h"
...
#include "widgetN_fwd.h"
#include "widgetfactory_fwd.h"

namespace Demo {
class WidgetFactory { ... };
class WidgetFactoryPrx { ... };
}

// File widgetfactory.cpp
#include "widget1.h"
...
#include "widgetN.h"
#include "widgetfactory.h"

namespace Demo {
// Implementation of WidgetFactory and WidgetFactoryPrx
}

As you see, the widgetfactory.h header does not include the definitions of all widgets, only forward declarations. Any users of WidgetFactory will be able to include the widget headers they actually need. widgetfactory.cpp does include full definitions and can contain marshalling code.

@bernardnormier
Copy link
Member

Thanks, I understand your proposal now. It would not change the Slice forward-declaration rule, but it would change the code (and files) generated by slice2cpp.

Note that slice2cpp cannot simply generate #include <xxx_fwd.h> in header files whenever it sees #include <xxx.ice>. For example:

// in file widgetfactory.ice

#include <widget.ice>
module Demo
{ 
   interface DerivedWidget extends Widget
   {
      void someOp();
   }

   interface WidgetFactory
   {
        Widget create();
        DerivedWidget createDerived();
   }
}

So depending on how the Slice types in the included Slice files are used, slice2cpp would have to generate different C++ #includes in widgetfactory.h.

This gets even more complicated for indirect inclusions like:

// in file widgetfactory.ice

#include <file_that_includes_widget.ice>
module Demo
{ 
   interface DerivedWidget extends Widget
   {
      void someOp();
   }
   ...

Here, we'd need to generate #include <widget.h>, since file_that_includes_widget.h probably includes only widget_fwd.h.

@Lastique
Copy link
Author

Ok, I see. So we need a Slice extension then, something like #include_impl that I mentioned earlier. Or make forward declarations in Slice work again some other way.

@bernardnormier bernardnormier added this to the 3.7.3 milestone Jul 8, 2019
@pepone pepone closed this as completed in f035214 Jul 9, 2019
@bernardnormier
Copy link
Member

The fix, as suggested by Lastique in the comment above, was to add a new metadata cpp:source-include. You need to use this metadata when your Slice file marshals or unmarshals a class or interface that is only forward-declared and you use the Slice to C++11 mapping.

Without the proper include in the generated source file, this generated source file won't compile.

You can find an example in the new test committed by Jose:
https://github.com/zeroc-ice/ice/blob/3.7/cpp/test/Ice/objects/Test.ice

@Lastique
Copy link
Author

Lastique commented Jul 9, 2019

Thanks! Is this only supported in C++11 mapping or in C++03 as well?

@pepone
Copy link
Member

pepone commented Jul 10, 2019

This should work with all language mappings, cpp:source-include is only required by the C++11 mapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants