Skip to content

Conversation

@YuriUfimtsev
Copy link
Contributor

No description provided.

@mrbean-bremen
Copy link
Contributor

This looks useful (both the parser fix for enum class and the better warnings), are you going to recreate this?

@YuriUfimtsev
Copy link
Contributor Author

Yes, I will recreate this in the better described PR.
A strange instability in the generation was found and I closed this PR. But it turned out that this instability is not a regression and not a result of the given code.

@mrbean-bremen
Copy link
Contributor

A strange instability in the generation was found

Yes, that is something that I have heard about and that also troubles me. There are some fixes by @jbowler (related to sorting) in the qt6 branch (which probably should be merged into main soon, as it also works with Qt5), but appearantly that still does not fix the problem. Would be good to get a grip on that problem... Feel free to write an issue with a description of the instability.

@jbowler
Copy link
Contributor

jbowler commented Oct 28, 2023

The generator code used qSort and/or std::sort on pointers to classes, in particular AbstractMetaClass*, without specifying a third, comparator, argument. See:

https://stackoverflow.com/questions/7446743/sorting-vector-of-pointers

The immediate consequence of this is that the order of AbstractMetaClassList is unpredictable and not the same across different builds of the generator and maybe even different runs of the same generator executable. I tried to fix this by finding all the cases I could but didn't necessarily get them all.

In addition it's worth looking at AbstractMetaBuilder::classesTopologicalSorted in abstractmetabuilder.cpp. This implements a "topological" sort. I first saw the name in the UNIX "tsort" program but Knuth dedicates a few pages to it. The sort is a partial order and the initial order therefore determines the order of the output of the generator. I fixed the initial std::sort of pointers to use a stable_sort of the class names but even that wasn't enough to get consistent output (but I hadn't done all the changes in a single build.)

The function in question has a storage leak and, worse, it seems to have a use-after-free error when it removes a key from a hash at line 2522 (qt6 branch) while iterating over that hash with a QHashIterator; the code did actually crash when I changed it to a QMutableHashIterator. I had a rewrite somewhere but I gave up on it because I wanted to look at Knuth's algorithm; it seemed that a lot of the compute time in the generator was going in that function.

In any case use-after-free can certainly make the output of a program highly unpredictable :-)

@jbowler
Copy link
Contributor

jbowler commented Oct 28, 2023

Incidentally if anyone wants to rewrite the topological sort please remove all uses of "new" and "delete"; they are completely unnecessary.

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.

3 participants