It's not possible to know whether an optional element was set from the API of the generated code

[villevoutilainen]

The code generator puts _nil bool flags for optional elements into the PrivateDPtr class of a complex type. However, the generated getters for these elements return by value, and they don't take the internal _nil flags into account in any way. Therefore it's possible that an optional element wasn't in the soap message, but getting it returns a default-constructed data object for it. That is just wrong, because optional elements that hold numeric values are returned as holding zeroes, which may be valid. This is a data corruption problem in the worst case.

[Knarf64]

Thanks for the report I will dig into this issue.

[villevoutilainen]

I can probably cook up a patch before the end of the week. I wouldn't mind having guidance which approach to take: 1) just a pointer 2) something smarter than that 3) how smart? :) QScopedPointer<T, DoNothing>& (a do-nothing deleter) would be decent, since that wouldn't require heap allocation and it would prevent accidental misuses like deleting the returned pointer. The issue with that is that it can't be returned as value, since QScopedPointer isn't copyable. An alternative would be to return a simple handle class that stores the pointer and provides operator-> and operator*. Something like boost::optional would also be an option.

[villevoutilainen]

See villevoutilainen@9847f63
Feedback welcome.

[Knarf64]

Thanks for the patch!
I will give it a look when I'll have a moment, pretty busy these days, many thanks.
Regards.

[villevoutilainen]

Probably could use some cleanups here and there, some pointless QLatin1-whatever ctors used when a literal will do, unit tests to be added. But the gist of the issue is solved in it. ;)

[villevoutilainen]

I have since updated my fork with a bugfix for a regression I managed to make, and added unit tests. The remaining things are

1. bike-shedding the command-line argument, -optional-element-type is a bit long
   (and perhaps should be -getter-type)
2. cleaning up the code generation, if necessary.
   Functionality-wise it's good to go.

[villevoutilainen]

Ping. Any news on this?

[dfaure-kdab]

Sorry, we're both a bit too busy these days ;-}

Anyhow I just looked at the patch, and I have to admit that I'm not too keen on a dependency on boost, since many people don't have that (but since it's optional, that's OK).
The alternative you implemented is a raw pointer, but a method that returns a pointer to an int is actually quite odd. And the caller always wonder whether he's supposed to delete the pointer or not (obviously not, from the implementation, but the API doesn't make that clear).

How does boost::optional work? It makes a copy? Then QSharedPointer would be quite similar? We make a copy and return it, and it can be tested for null, and it's deleted when it goes out of scope?

The alternative is to generate some foo_isNil() getter for each optional value. Did you consider this too ugly because it doubles the number of methods? I guess the benefit of your solutions is that they force checking that the value is present (at least, you get a crash if you don't), while people might just forget to check _isNil and just use default-constructed values...

[villevoutilainen]

This is going to be a bit long-winded. :)

First of all, note that the current status quo is preserved by the patch. People who want to continue using default-constructed values can do so, and that's what happens by default.

boost::optional copies, yes. It's a discriminated union that has a buffer the size of the object held, and a flag indicating whether an object is actually constructed into the buffer. When an optional element is not set, no object is created or copied.

I am fully aware of the horrors of raw pointers. I did consider using a KDSoap-specific "dumb pointer" that doesn't convert to a raw pointer, but didn't pursue the idea since it requires
adding such a dumb-pointer as a utility type and using it.

QSharedPointer has the downside that it incurs a fair amount of overhead - it will dynamically allocate a type-erased deleter inside itself. And we would need to use a do-nothing deleter since the actual data objects are stored by-value inside the generated classes, so we can't share them, we need to either fake-share them or copy them. That's why I didn't go with QSharedPointer, it would be quite confusing to have it when we don't truly have shared semantics.

We can't use QPointer either, it requires a QObject. We can't use a QScopedPointer because it can't be returned by value, and we would still need to consider the fake-sharing/copying, and potentially have pointless dynamic allocations. I didn't want to use QVariant because using it for custom types requires metatype registration and I considered that too complex to be worth it.

And yes, I didn't want to sprinkle separate isNil() query functions all over it.

To summarize:
Using a raw pointer means we don't perform excessive copies. Using boost::optional means we do copy, but we don't need additional dynamic allocation for that. Copying into a dynamically allocated object that is then held by eg. QSharedPointer has the overhead the dynamic allocation. Using a do-nothing deleter has the overhead of type-erasure (dynamic allocation of the deleter, indirect call of the deleter). I think a decent alternative would be a dumb-smart-pointer. A potentially easy addition is generating comments into the generated code that say the ownership of the pointer is not transferred.

[dfaure-kdab]

Thanks, now I see the reason why boost::optional exists :-)

The overhead of the actual copying with QSharedPointer (and boost::optional) wouldn't be that bad since the "complex types" use refcounting (QSharedData). But indeed the overhead of the dynamic allocation inside QSharedPointer make it a bit worse than boost::optional. I wonder if it's not good enough though. Anyone who wants to optimize that away can install boost....

With that solution (copying into a QSharedPointer) I don't think it's confusing - the user of that pointer can decide to use the sharing feature of it, or not. It's not shared with the copy inside the generated class, but I don't see why this matters, we make no such promise, just by the fact that we're returning a QSharedPointer.

In any case, I agree that the raw-pointer solution can do then, with an additional comment generated in the method documentation.

And yes, I know and agree that the current default behavior stays, for source-compatibility reasons.

[villevoutilainen]

Ok. I'll take a look at the comment-generation, and will take a stab at a QSharedPointer option. I'd like to keep the raw-pointer and boost::optional options available just in case, although we're on the road to creeping featurism. I'll ping again when I have a new patch. ;)

[dfaure-kdab]

Another option might be std::shared_ptr with std::make_shared, which my colleague says, "uses
perfect forwarding to create the payload instance as part of the control block allocation, saving one heap allocation". However that might only be true for C++11?

[villevoutilainen]

Correct, std::shared_ptr is C++11. make_shared indeed allocates the payload and the control block in one go. It's just probably a tad too new to be "THE" option. :)

[villevoutilainen]

And, by the way - using QSharedData does not really help here. QSharedData works with QSharedDataPointer when allocations are dynamic. It doesn't magically prevent a by-value member inherited from QSharedData from being destroyed when there are still QSharedDataPointers pointing to the member.

[mbroadst]

fwiw there was a huge discussion about adding a QOptional template class to Qt over here (http://lists.qt-project.org/pipermail/development/2014-February/015422.html). This was originally discussed for all the toInt, toLong, etc for QString, but would also have applied to changes in QtDBUS needed to support kdbus. Thiago gave up on it due to time constraints for 5.3 iirc.

Do you really need to add boost::optional, or could you just provide a "bool *ok = 0" for the accessor, and then provide an "isValid" or "isNull" on the returned type? Excuse me if I sound like I totally don't understand what I'm talking about, I just very quickly read through all this :)

[villevoutilainen]

Well, we could have either a bool* ok = 0 for the accessor or provide an isNull for the returned type. I see little value in combining those approaches. The former has the disadvantage that it constructs a value even if it's not present. The latter has the same disadvantage, the object is still constructed but is not valid, so it can be misused rather easily. I'd actually prefer undefined behaviour when mistakes are made, because there are tools that can detect and diagnose UB, and on most platforms dereferencing an unset boost::optional, a null pointer or a null QSharedPointer is loud and clear and visible. I added boost::optional as an alternative because it's a very handy type to express these things for users who can afford a boost dependency (and that dependency is completely optional (no pun intended), you need to separately ask for boost::optional to get it).

[dfaure-kdab]

Re earlier comments, I agree that providing both raw pointers and QSharedPointer is too much, we should decide on one or the other. And since you convinced me that a raw pointer will always be more efficient, maybe let's stick to that.

QSharedData (which is already used, I'm not suggesting that you do anything about that) helps in the sense that the "d" pointer of all generated "value classes" uses it, making copies cheap (just refcounting). Just like e.g. QString does. The value class itself doesn't inherit QSharedData, its private class does. (all this at the price of an extra dynamic allocation when first creating the value, of course).
Now that I think about it, in Qt this is necessary to be able to extend classes for binary compatibility reasons, while in kdsoap's generated code we don't have that constraint, so maybe it shouldn't generate a separate PrivateDPtr class....

bool *ok sounds even less convenient to me than separate foo_isNil() accessors, in terms of the application code (3 lines: declaring bool, calling getter, testing bool, instead of 2 lines: testing isNil, calling getter).

isNull for the returned type doesn't work for e.g. int.

Matt, are you saying that it's likely that Qt will get a QOptional template in the future? In that case, we'll be able to easily add support for that, based on the boost::optional code, which is quite nice.

[villevoutilainen]

David, it seems like you're more or less saying that the patch is good as-is. :)

Adding QOptional support is indeed easy - as is adding support for any kind of "nullable proxy". The difference between the raw pointer support and boost::optional support is rather small, and adding more such types is quite straightforward.

[dfaure-kdab]

Good as is: yes, apart from the missing note in the generated documentation, so people don't try to delete that pointer.

[villevoutilainen]

Ah, yes. I'll add that. :)

[mbroadst]

Yeah I agree using QOptional would be the best approach (I mean boost::optional is the ideal here, but we don't want to depend on boost where we can get away with it, and in my case that's: everything I work on involving Qt ;) ). Unfortunately, the takeaway from that email chain was that thiago had written a basic implementation, and then the list bike shedded on it for a while and he got fed up and left it as "well, hopefully this gets in later," which of course depends on someone having the effort to modify all the relevant unit tests etc.. unfortunately one of the less desirable sides of open source.

Having said that, something that provides this functionality is a requirement for kdbus support, which will be a requirement in Qt at some point. I think your raw pointer solution is fine for now, and since these are generated classes it will be easy to provide a pivot (maybe through a command line option to kdwsdl2cpp or something) in the future. Of course another option is to take the code that he posted in the various codereview links posted in that mailinglist, adding QOptional to KDSoap now and doing the hard work there to prove it works for later upstream inclusion in qt. Again, it's up to how much time you all have and whether you're inclined to do the work :)

[villevoutilainen]

Patch done, pull request created: #44

[villevoutilainen]

Ping, any plans to merge this?

[dfaure-kdab]

Thanks for the patches. I cherry-picked them, fixed formatting a little bit (ambiguous else, trailing spaces), renamed the unittest subdir (issue43 isn't very descriptive - I know, there are others like that, I'll rename them too).

Once I push it, I expect our CI to fail on the boost requirement (from the unittest) on some systems, let's see if we have to write a check for the boost headers being available.

Finally I noticed that a test for the optional value being not present is missing. It should then be empty (in "regular"), 0 (in "pointer") and whatever the boost API is for not-present in "boost-optional". Feel like adding that to the unittests? (on an updated clone please, so it applies to upstream kdsoap) ;)

[villevoutilainen]

A test for the optional value being not present is missing? The tests do two compares, one for the case where the optional value is not set, one for the case where it is set.

[dfaure-kdab]

Ah yes, OK, sorry.

OK, the build fails on Windows with
Cannot open include file: 'boost/optional.hpp': No such file or directory

Up for a "configure" check?

[villevoutilainen]

Well, the snag about that is that I don't have a development-capable windows environment at hand. Unless you just want to disable that test on windows...

[dfaure-kdab]

Uninstalling the boost/optional.hpp header on linux would do :-)

This isn't windows-specific, it's about not breaking the build on any system without boost/optional.hpp available.

This is possibly tricky to do in qmake (and very easy in cmake...).

Since we have a layer about qmake (the configure script), we could also compile a test program there....

[villevoutilainen]

Ok, I'll take a look. May take a bit of time.

[villevoutilainen]

RFC-question: instead of trying to detect boost (which may be just anywhere on windows) or trying to compile a test application (with what make? what compiler? what makespec?), how about just adding a new flag to the configure.{sh,bat} that enables the boost-depending unit test? Yes, it's not pretty, but none of the other options seem very pretty either. And.. this is partly windows-specific because our "layer above qmake" is either a shell script or a bat, depending on the platform, sadly.

[dfaure-kdab]

"what make+compiler+makespec" -> well, the same as the ones that are going to be used to compile the code, so that part should be no problem.

But yeah, I can see how it's a lot easier to add an option to configure.sh/.bat. I consider this inferior, but I'm happy to blame qmake for that :)
Let's go for that, to resolve the situation faster.

[villevoutilainen]

Yeah, of course same ones that are going to be used to ultimately build, but how do I know which ones those would be in configure.bat, that's the puzzle. :) There are perhaps alternatives to the blunt instrument that is adding an option to configure, like
http://qt-project.org/doc/qt-4.8/qmake-function-reference.html#packagesexist-packages

[dfaure-kdab]

You would just run qmake[.exe] on a .pro file in a subdir, just like configure.bat/.sh runs qmake[.exe] further down for the actual project. OK the only additional thing would be to call nmake, mingw-make or gmake/make afterwards (on that test project), which isn't currently done by configure. Indeed I'm not sure how you could figure out the name of the "make" program to use. This is the only hurdle, we know qmake and qmake knows the makespec and the compiler.

packagesExist is based on pkgconfig, and boost-devel doesn't install a pkgconfig ".pc" file here. So it can't be used for this purpose.

[villevoutilainen]

Yep, boost packages don't seem to use pkgconfig. By the way, qmake thinks it knows the compiler/make. That doesn't mean it really knows that, if you happen to use something else than msvc, I guess. :) Let's go with the dumb-as-dog**** configure option for now...

[dfaure-kdab]

BTW we have now created a mailing-list where users and developers of KDSoap can discuss issues.
You can subscribe at https://mail.kdab.com/mailman/listinfo/kdsoap-interest

This list got a post today from Nicolas Beudez who had an issue with the raw-pointer option. Maybe you can take a look at it? (you can read it in the archives)
Thanks!

[villevoutilainen]

I tried subscribing, everything seemingly went well but I can't access the archives, the mailing list interface insists that authentication failed.

[dfaure-kdab]

I approved the subscription, should be ok now.