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

Utilize the -no-sync/async/async-jobs arguments in unittests and examples #230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martonmiklos
Copy link
Contributor

As we discussed in #225 I have added the new no-sync/async/async-jobs arguments to the unittests and examples project files. I have update the qmake project files as well.

I found a few qmake pro files where they were not in sync with the cmake one (they lacked the -use-local-files argument), and I have removed some Qt4 legacy conditions in the examples pro files.

There are a couple of unit tests which only validates the generated code compile-ability. (vidyo for e.g.)
I was hesitating what to do with them but in these cases I turned off all the 3 type API generation.

As a side note: in my setup the compile time reduction was not significant (2 min 59 s with this PR whilst 3 min 7 sec with the master).

@dfaure-kdab
Copy link
Member

"I turned off all the 3 type API generation." ... what gets generated, then?
For those I would rather turn ON all three....

@martonmiklos
Copy link
Contributor Author

martonmiklos commented May 25, 2021

Let's dust this a little bit off before the release to see if we can put this to the next release.

"I turned off all the 3 type API generation." ... what gets generated, then?

The classes representing the WSDL datatypes will get generated, only the call handling slots/signals and the KDSoapJob
subclass won't be generated.
Here is an example of the generated codes difference: https://gist.github.com/martonmiklos/cf8ab586aeba9dffd3162fca1f9d6ece/revisions

But these calls/class is not utilized in those tests, for me the purpose of these tests looks like to validate that the code compiles when including the generated codes.

All the three approaches are used within other unit tests so the code generator output compile-ability is verified for each type I think, but I am not against turning back on all the three for the tests in question.

@dfaure-kdab
Copy link
Member

Ah, I see. It's pretty hard for me to determine if disabling some parts of the code generation might one day hide a regression in kdwsdl2cpp that would otherwise have been detected by a compile error.

Yes, various tests end up testing the three modes, but if some specific feature generates wrong code in a specific mode, we might then miss a test for that combination....

@martonmiklos
Copy link
Contributor Author

martonmiklos commented Oct 23, 2021

Ah, I see. It's pretty hard for me to determine if disabling some parts of the code generation might one day hide a regression in kdwsdl2cpp that would otherwise have been detected by a compile error.

Yes, various tests end up testing the three modes, but if some specific feature generates wrong code in a specific mode, we might then miss a test for that combination....

I understand these concerns. I think you need to estimate how much added value has the lowering of the compilation tests. If low then leave as it is. I will still sleep well if you say that we should rather close and leave this change. ;-)

@dfaure-kdab
Copy link
Member

A good compromise is probably to

  • generate as much code as possible for most tests
  • test the -no-async/async/async-jobs arguments in just a few tests
    Testing that the async code was NOT generated is a bit tricky though. Interesting exercise in SFINAE... :) Feel free to ignore that part :)

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.

2 participants