Skip to content

Comments

Addresses some problems found while trying to use Pool::componentsByCategories#441

Merged
ximion merged 3 commits intoximion:masterfrom
aleixpol:work/fix-qt
Nov 1, 2022
Merged

Addresses some problems found while trying to use Pool::componentsByCategories#441
ximion merged 3 commits intoximion:masterfrom
aleixpol:work/fix-qt

Conversation

@aleixpol
Copy link
Collaborator

It was crashing because the list was not getting populated properly. This changes does so and addresses a couple of extra problems found while doing this as seen in their respective commits2.

Push before popping or the compiler complains
@aleixpol aleixpol requested a review from ximion October 25, 2022 15:56
@ximion
Copy link
Owner

ximion commented Oct 26, 2022

The patches make sense to me except for the qt: Fix Pool::componentsByCategories one - which bug does that one fix?
Using g_new0 already ensures that the resulting memory is nulled, so no need to do that explicitly.
Did the strings from categories somehow have a shorter life than the surrounding container?

@aleixpol
Copy link
Collaborator Author

Did the strings from categories somehow have a shorter life than the surrounding container?

Note qPrintable is the result of .toUtf8().constData(). We could consider splitting it into two loops, one converting to QByteArray and the other creating the gchar **. We'd save a few memcpy() calls.

@ximion
Copy link
Owner

ximion commented Oct 27, 2022

Sure, but does that mean the original bug was indeed the categories being destroyed before the function returned? That would be extremely surprising to me!
Using the deep-copy stringListToCharArray here is probably fine though, as the additional memory allocations won't cause a bug performance hit (the categories array is quite small, and the few nanoseconds saved for allocating & copying a few strings are likely unnoticeable. It's not like we are running this in a loop thousands of times)

Make sure the list is NULL-terminated as expected
Makes sure we don't use a string that is out of scope, keeps the UTF-8
formatted array until affer as_pool_get_components_by_categories is
called.
@aleixpol
Copy link
Collaborator Author

If I don't have this commit I sometimes get this error message:
** (process:152147): WARNING **: 15:14:03.557: '' is not a valid XDG category name, search results might be invalid or empty.

I've changed the disputed commit to the slightly more verbose but correct and efficient version.

@aleixpol
Copy link
Collaborator Author

Ping?

@ximion
Copy link
Owner

ximion commented Nov 1, 2022

If I don't have this commit I sometimes get this error message: ** (process:152147): WARNING **: 15:14:03.557: '' is not a valid XDG category name, search results might be invalid or empty.

Really strange! This does point at some kind of lifetime issue though.

I've changed the disputed commit to the slightly more verbose but correct and efficient version.

I would have even been fine with the previous version, but I do need to understand the rationale behind a change (merging blindly without understanding the cause will eventually haunt you).
But the new version is fine too :-)

Thank you for the patch!

@ximion ximion merged commit 8c9044c into ximion:master Nov 1, 2022
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