Re: Review Request 114448: kjs: reogranize basic math functions & function checking
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114448/#review102638 --- The patch was not commited 3 years ago. It still applies but given kdelibs is in heavy-important-bugfixes only mode i guess we should discard this? Will do in two weeks unless someone disagrees. - Albert Astals Cid On Dec. 14, 2013, 4:30 p.m., Bernd Buschinski wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/114448/ > --- > > (Updated Dec. 14, 2013, 4:30 p.m.) > > > Review request for kdelibs. > > > Repository: kdelibs > > > Description > --- > > kjs: reorganize basic math functions & function checking. > Most noticeable change is that kjs will no longer quietly fail at runtime > if a important function is missing, but fail at compiletime. > This will only happen on a very strange platform, or a typo in my checks. > Also note, I added more checks, so theoretically more platforms are now > supported. > > Ah yes and the math functions are now always inlined, not that it makes any > huge performance difference... > > > Diffs > - > > khtml/ecma/kjs_arraybuffer.cpp c4d2e51 > khtml/ecma/kjs_arraybufferview.h 4b6992b > khtml/ecma/kjs_context2d.cpp bbdba8f > kjs/CMakeLists.txt 48f5231 > kjs/config-kjs.h.cmake f644528 > kjs/date_object.cpp c8d776c > kjs/function.cpp 1102208 > kjs/internal.cpp 49c2ce2 > kjs/jsonstringify.cpp e07a789 > kjs/math_object.cpp 89835e5 > kjs/number_object.cpp c284746 > kjs/operations.h 1112c2b > kjs/operations.cpp 9e2fc71 > kjs/value.cpp f02aeea > kjs/wtf/MathExtras.h 58be75b > > Diff: https://git.reviewboard.kde.org/r/114448/diff/ > > > Testing > --- > > > Thanks, > > Bernd Buschinski > >
Re: Review Request 113152: kcm_clock: Check for valid return values of QDateTime::toTime_t()
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/113152/ --- (Updated Feb. 26, 2017, 12:37 a.m.) Status -- This change has been discarded. Review request for kde-workspace. Repository: kde-workspace Description --- Using the date selector in kcm_clock to set any date further than February 7, 2106 resulted in setting the time to ((time_t) (unsigned int) -1). This patch makes setting any date further than this to spit out an DateError. Also, it's a seed for discussion regarding this KCM module as passing the values as QStrings and converting from/to time_t doesn't seem like a manageable solution to the problem. Thanks for your opinions. Diffs - kcontrol/dateandtime/dtime.cpp 518afe5 kcontrol/dateandtime/helper.cpp 9168db3 Diff: https://git.reviewboard.kde.org/r/113152/diff/ Testing --- Built and tested on Fedora 19 x86_64. Thanks, Martin Bříza
Re: Review Request 113152: kcm_clock: Check for valid return values of QDateTime::toTime_t()
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/113152/#review102632 --- Patch wasn't commited for years so i guess it wasn't that important. Discarding. - Albert Astals Cid On Oct. 7, 2013, 4:06 p.m., Martin Bříza wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/113152/ > --- > > (Updated Oct. 7, 2013, 4:06 p.m.) > > > Review request for kde-workspace. > > > Repository: kde-workspace > > > Description > --- > > Using the date selector in kcm_clock to set any date further than February 7, > 2106 resulted in setting the time to ((time_t) (unsigned int) -1). This patch > makes setting any date further than this to spit out an DateError. > Also, it's a seed for discussion regarding this KCM module as passing the > values as QStrings and converting from/to time_t doesn't seem like a > manageable solution to the problem. > Thanks for your opinions. > > > Diffs > - > > kcontrol/dateandtime/dtime.cpp 518afe5 > kcontrol/dateandtime/helper.cpp 9168db3 > > Diff: https://git.reviewboard.kde.org/r/113152/diff/ > > > Testing > --- > > Built and tested on Fedora 19 x86_64. > > > Thanks, > > Martin Bříza > >
Re: Review Request 127866: Oxygen: Fix QCache usage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/ --- (Updated Feb. 26, 2017, 12:28 a.m.) Status -- This change has been marked as submitted. Review request for kde-workspace and Hugo Pereira Da Costa. Repository: oxygen Description --- This should mostly complete the QCache fixes I kicked off in a previous RR, 127837. Hugo noted there were many other similar usages, and boy he wasn't kidding! ;) The long story short is that these usages can theoretically cause use-after-free behavior (which can lead to crashes and even undefined behavior if the compiler ever gets smart). *NOTE* It is -much- easier to review if you download the diff to your git repository for oxygen and then run "git diff -b" to ignore whitespace changes, particularly for the QPixmap changes. For QPixmaps we return values instead of pointers, so we simply make a separate copy to be cached when we do insert. For QColor we return references to values so we *must* return pointers, and those have to be owned by a QCache to avoid memleaks. So I added a helper function to loop until the cache accepts the new entry. TileSets are a similar concern, except those have manual loops since I was uncertain about whether TileSet's copy constructor was the best idea or not. This fixes a ton of Coverity issues (59717 - 259733, 259739, 259742 - 259752, 1336154, 1336155) and might be associated with Qt bug 38142 and KDE bug 219055 (which doesn't actually appear to be a dupe of a different bug to me...). Diffs - kdecoration/oxygendecohelper.cpp aa75eca kstyle/oxygenstyle.cpp e428606 kstyle/oxygenstylehelper.h 9510a60 kstyle/oxygenstylehelper.cpp 612ba37 liboxygen/oxygenhelper.h a6453a0 liboxygen/oxygenhelper.cpp 4843604 liboxygen/oxygenshadowcache.cpp 907e586 Diff: https://git.reviewboard.kde.org/r/127866/diff/ Testing --- Compiled without warnings, installed and ran `oxygen-demo5 -style oxygen`. Used the GUI Benchmark feature to automatically cycle through all the listed features -- no crashes or obvious rendering errors. Thanks, Michael Pyne
Re: Review Request 125081: Fix build on OS X
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125081/ --- (Updated Feb. 26, 2017, 12:26 a.m.) Status -- This change has been discarded. Review request for KDE Base Apps and Martin Gräßlin. Repository: kde-baseapps Description --- Use kwindowsystem.h to check whether X11 is available and move netwm.h with them since that header is made available with the xcb plugin. added .reviewboardrc Diffs - .reviewboardrc PRE-CREATION konqueror/src/konqmainwindow.cpp c7a81c8 Diff: https://git.reviewboard.kde.org/r/125081/diff/ Testing --- Build on OS X 10.8 Thanks, Samuel Gaist
Re: Review Request 125081: Fix build on OS X
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125081/#review102629 --- Since Samuel did not respond to the improvement requests by Martin i guess we can discard this 1.5 years old review. - Albert Astals Cid On Sept. 7, 2015, 10:08 p.m., Samuel Gaist wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125081/ > --- > > (Updated Sept. 7, 2015, 10:08 p.m.) > > > Review request for KDE Base Apps and Martin Gräßlin. > > > Repository: kde-baseapps > > > Description > --- > > Use kwindowsystem.h to check whether X11 is available and move netwm.h with > them since that header is made available with the xcb plugin. > > > added .reviewboardrc > > > Diffs > - > > .reviewboardrc PRE-CREATION > konqueror/src/konqmainwindow.cpp c7a81c8 > > Diff: https://git.reviewboard.kde.org/r/125081/diff/ > > > Testing > --- > > Build on OS X 10.8 > > > Thanks, > > Samuel Gaist > >
Re: Review Request 122252: KRecursiveFilterProxyModel: fix emitting superfluous dataChanged signals
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122252/#review102624 --- No progress here in 2 years and kdelibs is in very-life-support, should it be discarded? - Albert Astals Cid On Jan. 25, 2015, 6:51 p.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122252/ > --- > > (Updated Jan. 25, 2015, 6:51 p.m.) > > > Review request for kdelibs and Christian Mollekopf. > > > Repository: kdelibs > > > Description > --- > > by using an internal cache of the filtering state. > > The tricky part is that filterAcceptsRow() must not use the cache > (too bad, it would be faster), because of the setFilterFixedString() > feature which can change the filtering status of indexes without > KRFPM even being informed (QSFPM has no virtual method, no event...) > So it only ever writes to the cache, but when dataChanged() > or row insertion/removal comes in, we can look into the cache > to find the old state and compare. > > > Diffs > - > > kdeui/itemviews/krecursivefilterproxymodel.h > c16b62186fb9203ff297bd9fd28d9c13a1c8bdc4 > kdeui/itemviews/krecursivefilterproxymodel.cpp > efa286ad87ded962b20c8a581b659d1b154ebf3a > kdeui/tests/krecursivefilterproxymodeltest.cpp > 3bcb72980730cb22f887ae8fa5fbd91b5609aeb6 > > Diff: https://git.reviewboard.kde.org/r/122252/diff/ > > > Testing > --- > > Unit tests. > > Using kmail (and especially testing the substring filtering in the "move > dialog", which an earlier version of this patch broke). > Looking at the number of calls to > Akonadi::FavoriteCollectionsModel::Private::reload() for a single > dataChanged() signal from the ETM, which went from 10 to 4 (still too many, > but the remaining problem is elsewhere). > > > Thanks, > > David Faure > >
Re: Review Request 127866: Oxygen: Fix QCache usage
> On May 30, 2016, 11:34 a.m., Hugo Pereira Da Costa wrote: > > Ship It! > > Hugo Pereira Da Costa wrote: > err. Wait ... > There are rendering issues here once the patch is applied. > See http://wstaw.org/m/2016/05/30/plasma-desktopY12228.png > (left is "before", right is "after"). > So something seems wrong with the background gradient. > I'll investigate a bit ... > > Hugo Pereira Da Costa wrote: > Hi again, > So, thinking more about it, and actually answering the questions raised > in the review: > > - do we track public API for this part of Oxygen? Does anything in a > different library or application link to this? > No we don't this is supposed to be an "internal" (as in private) library, > used only by oxygen style and decoration. No ABI/API guarantee. > > - QColor: can we change the return type. I would say yes, (from reference > to value), and return to using QCache > - Tilesets: I would be inclined to changing the return type here too, > using values, and assuming the copy constructor does not cost much, based on > the implicit-shareness nature of pixmaps, and keep a built-in QCache here > (which should be more efficient than the (otherwise nice) FIFO. > > Now I understand that this is a lot of going back and forth, and a job I > should rather do myself, as the maintainer of the code. > So I would propose to "postpone" this RR for now, and for me to locally > - take the QPixmap change from this patch > - implement something similar for QColor and TileSet > - test. > Then if I manage to do that in a reasonable time (e.g. this week), drop > the review and commit my change instead (with proper credits where due). > Otherwise (because of me being too busy with other stuff), just commit this > review (once the problem mentionned above is fixed, though I could not > investigate yet). > > What do you think ? > > (also: I need to sanitize this run-time changing of the cache use and > max-cost) > > Michael Pyne wrote: > Hugo, > > That all sounds fair. And if it's all too much difficulty, then we might > be able to lean on the hinted-at-but-not-quite-documented QCache behavior > that ::insert() only fails if the item being inserted has a cost higher than > maxCost. In that case I could ignore the Coverity issues (since Coverity > can't statically prove that items are always < maxCost) and then drop the RR > (and perhaps ask Qt to document more stringently that guarantee for > QCache::insert). > > Still though, I think the QColor return type change would make sense even > independent of this RR. > > Hugo Pereira Da Costa wrote: > Hi again Michael, > > So, I have a working implementation here that > - keeps using QCache wherever possible, passing QColor and TileSet as > values rather than as refs (in a way that is similar to the change you did > for QPixmaps) > - uses your FIFOCache for the implementation of Oxygen::Cache, which is a > "cache of caches", and thus cannot possibly use values. > > Seems to work (though I would like to test a bit more), and should fix > all the issues with Coverty. > Since I have blatently copied your code for the home-made FiFOCache, i > have added you as a copyright holder for oxygenhelper.h > is that ok with you ? Should i put your name in other places too ? > > Best, > > Hugo > > Michael Pyne wrote: > > Since I have blatently copied your code for the home-made FiFOCache, i > have added you as a copyright holder for oxygenhelper.h > is that ok with you ? Should i put your name in other places too ? > > That's perfectly fine. You don't need to mention me elsewhere, as long as > this RR is mentioned in the commit log I think that would cover any needed > attribution of work. > > Albert Astals Cid wrote: > What's the status of this, was commited? Needs further work? Should be > discarded? yes.Code was committed,review should be discarded - Hugo --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/#review96029 --- On May 22, 2016, 4:20 a.m., Michael Pyne wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127866/ > --- > > (Updated May 22, 2016, 4:20 a.m.) > > > Review request for kde-workspace and Hugo Pereira Da Costa. > > > Repository: oxygen > > > Description > --- > > This should mostly complete the QCache fixes I kicked off in a previous RR, > 127837. Hugo noted there were many other similar usages, and boy he wasn't > kidding! ;) The long story short is that these usages can
Re: Review Request 127866: Oxygen: Fix QCache usage
> On May 30, 2016, 11:34 a.m., Hugo Pereira Da Costa wrote: > > Ship It! > > Hugo Pereira Da Costa wrote: > err. Wait ... > There are rendering issues here once the patch is applied. > See http://wstaw.org/m/2016/05/30/plasma-desktopY12228.png > (left is "before", right is "after"). > So something seems wrong with the background gradient. > I'll investigate a bit ... > > Hugo Pereira Da Costa wrote: > Hi again, > So, thinking more about it, and actually answering the questions raised > in the review: > > - do we track public API for this part of Oxygen? Does anything in a > different library or application link to this? > No we don't this is supposed to be an "internal" (as in private) library, > used only by oxygen style and decoration. No ABI/API guarantee. > > - QColor: can we change the return type. I would say yes, (from reference > to value), and return to using QCache > - Tilesets: I would be inclined to changing the return type here too, > using values, and assuming the copy constructor does not cost much, based on > the implicit-shareness nature of pixmaps, and keep a built-in QCache here > (which should be more efficient than the (otherwise nice) FIFO. > > Now I understand that this is a lot of going back and forth, and a job I > should rather do myself, as the maintainer of the code. > So I would propose to "postpone" this RR for now, and for me to locally > - take the QPixmap change from this patch > - implement something similar for QColor and TileSet > - test. > Then if I manage to do that in a reasonable time (e.g. this week), drop > the review and commit my change instead (with proper credits where due). > Otherwise (because of me being too busy with other stuff), just commit this > review (once the problem mentionned above is fixed, though I could not > investigate yet). > > What do you think ? > > (also: I need to sanitize this run-time changing of the cache use and > max-cost) > > Michael Pyne wrote: > Hugo, > > That all sounds fair. And if it's all too much difficulty, then we might > be able to lean on the hinted-at-but-not-quite-documented QCache behavior > that ::insert() only fails if the item being inserted has a cost higher than > maxCost. In that case I could ignore the Coverity issues (since Coverity > can't statically prove that items are always < maxCost) and then drop the RR > (and perhaps ask Qt to document more stringently that guarantee for > QCache::insert). > > Still though, I think the QColor return type change would make sense even > independent of this RR. > > Hugo Pereira Da Costa wrote: > Hi again Michael, > > So, I have a working implementation here that > - keeps using QCache wherever possible, passing QColor and TileSet as > values rather than as refs (in a way that is similar to the change you did > for QPixmaps) > - uses your FIFOCache for the implementation of Oxygen::Cache, which is a > "cache of caches", and thus cannot possibly use values. > > Seems to work (though I would like to test a bit more), and should fix > all the issues with Coverty. > Since I have blatently copied your code for the home-made FiFOCache, i > have added you as a copyright holder for oxygenhelper.h > is that ok with you ? Should i put your name in other places too ? > > Best, > > Hugo > > Michael Pyne wrote: > > Since I have blatently copied your code for the home-made FiFOCache, i > have added you as a copyright holder for oxygenhelper.h > is that ok with you ? Should i put your name in other places too ? > > That's perfectly fine. You don't need to mention me elsewhere, as long as > this RR is mentioned in the commit log I think that would cover any needed > attribution of work. What's the status of this, was commited? Needs further work? Should be discarded? - Albert --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/#review96029 --- On May 22, 2016, 4:20 a.m., Michael Pyne wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127866/ > --- > > (Updated May 22, 2016, 4:20 a.m.) > > > Review request for kde-workspace and Hugo Pereira Da Costa. > > > Repository: oxygen > > > Description > --- > > This should mostly complete the QCache fixes I kicked off in a previous RR, > 127837. Hugo noted there were many other similar usages, and boy he wasn't > kidding! ;) The long story short is that these usages can theoretically cause > use-after-free behavior (which can lead to crashes and even
