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

kdwsdl2cpp: Avoid potential type collisions in nested complexTypes #91

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hkaelber
Copy link

So far, for complexTypes nested in elements with the same name a
single C++ class has been created. This is wrong in cases where
multiple elements with the same name contain complexTypes of different
structure because different classes should be created

#82

So far, for complexTypes nested in elements with the same name a
single C++ class has been created. This is wrong in cases where
multiple elements with the same name contain complexTypes of different
structure because different classes should be created
Copy link
Member

@dfaure-kdab dfaure-kdab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch!

I had a look at its effect on other WSDL files, and I wonder if other users might see this as a regression when the element being defined multiple times, is actually defined with the same contents.

For instance, logbookifv3.wsdl says

          <s:element minOccurs="0" maxOccurs="1" name="records">
              <s:complexType mixed="true">
                <s:sequence>
                  <s:any />
                </s:sequence>
              </s:complexType>
            </s:element>

in multiple places, and any TNS__Records instance would be usable in all setRecords methods. This patch creates three records classes, incompatible with each other at the C++ level even they say the same.

For perfection and to avoid breaking existing code, this means we should actually compare content and use the same name for the same content. Should be "easy", it just requires implementing a number of operator==() in schema/*.

What do you think ?

@hkaelber
Copy link
Author

Thanks for the comment, I agree.

I pushed a commit, that implements some operator==().

I updated the test-cases a bit and made sure that your example from logbookifv3.wsdl is covered and did some tests with our cisco AXL wsdls.

I decided to define equality with respect to the corresponding data-structures/the XML schema definitions and not the resulting generated classes. That means that there can be cases of nested complex types that are considered inequal due to some properties (like i.e. Element.min/maxOccurs()) that lead to the same generated classes.

After some testing with the gigantic cisco AXL API, this seems to be ok, because most such cases that I saw seemed to have little impact with respect to generated code size.

If you want to further optimize this, pls point me to the properties that I could probably drop from the operator==().

Nonetheless, as this further bloats the generated code (in case of the AXL API v. 11.0 to > 25MB, which does not compile on my 4 gig RAM machine without swapping :-( I'd love to add some --whitelisted-operation="op1,op2,..." parameter to kdwsdl2cpp to add a means to limit the size of generated code by telling it which operations should be generated. Some quick tests looked promising. Comments from your side?

Thanks,
Holger

@dfaure-kdab
Copy link
Member

Wow I seem to have completely missed that last comment, in almost two years... Email overload.

I like the idea of specifying which operations to generate, for the case of a huge wsdl file where only some operations are wanted. Maybe instead of a (huge) command-line argument it could be a sort of "config file" (better suited for the case of a very long list of operations). This would also open up the possibility of manually specifying namespace prefixes in that same config file, which is something others have asked about as well. This is something that can of course be implemented completely separately from what is being discussed here.

But I wonder if by now you're still interested... really sorry about the huge delay in my reply.

@raphaelcotty
Copy link
Contributor

I would be interested by the patch.
I see that most of the changed now belong to the https://github.com/cornelius/libkode repository.
Would that make sense if I try to reactivate the patch?

@dfaure-kdab
Copy link
Member

Yes it would certainly make sense to rebase/redo the patch on top of the libkode repository.

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

Successfully merging this pull request may close these issues.

None yet

3 participants