search

mbasaglia / Qt-Color-Widgets

Color wheel widget and dialog for Qt
147 stars 54 forks source link

quality: Fix some warnings #22

Closed Elv13 closed 7 years ago

Elv13 commented 7 years ago

There is still many, many more, but at least this covers most of Qt Clazy level 1 and some of the auto-fixit from level2.

Also disables -Werror. Werror should be a local CXXFLAGS/CFLAGS, not a project one. This break, among other things:

callaa commented 7 years ago

There's a bug in the commit. In color_palette.cpp, QHash<QString,QString> is changed to QHash<QString,QStringRef>. However, the QString object referenced by the QStringRef will be deleted on the next iteration of the loop.

Elv13 commented 7 years ago

But wont the hash be long gone by then and the string "re-referenced" when calling setName()

callaa commented 7 years ago

I'm not sure what you mean. When setName(properties[QStringLiteral("name")].toString()) is called, the returned QStringRef instance will point to memory freed earlier on line 216. QStringRef does not increment a string's reference count and so is only valid when the string it is referencing still exists.

Elv13 commented 7 years ago

Right, I will report a bug to Clazy about the false positive. They list this one as 100% safe and false positive free. I force pushed a fix

mbasaglia commented 7 years ago

I believe this would break Qt 4 support, right? It isn't an issue, but I'll have to keep in mind to remove the Qt4 checks if this is the case.

Elv13 commented 7 years ago

I don't mind using a fork either. I guess merging this would cause more issues than it fixes. Shall I close this? The only reason this exists is to lower the warning count in my CI. Other than that, it is 0.1% faster, but who cares.

mbasaglia commented 7 years ago

Nah, it's fine. I'll have to check that everything still works for me though which might take a couple of days.

mbasaglia commented 7 years ago

Looks good to me