Re: [Qgis-developer] Q/A Compiler Warnings
On 06/22/2015 02:03 PM, Nyall Dawson wrote: > On 16 June 2015 at 21:54, Matthias Kuhn wrote: >> In the quest for an always closer-to-perfection state of code, some >> goblins tried to get the warning count (on gcc and clang) yesterday as >> close to zero as possible. This was done in order to allow you (as a >> developer) to see if your code changes cause warnings without scrolling >> through heaps of meaningless warnings. > Thanks goblins! Your work in much appreciated :) > > >> At the moment most of the warnings left originate from the geometry >> refactoring and point to empty methods which have not yet been >> implemented. These warnings point to real problems and should be fixed >> as soon as possible, in order to have some time left before release to >> be able to test the fixes before the release. @Marco, what is your >> schedule concerning these? > I've just pushed some fixes to master which reimplement these stubbed > methods and comment out some of the unimplemented new methods. 2.10 > release was approaching too close for comfort to leave these in! > @Marco Please let me know if I've made any errors doing this... Thanks Nyall. With a warning count of 0... > > >> The plan is to let travis test for warnings and if there are warnings >> mark the build red. With this in place, an author of a commit will be >> required to spend some thoughts on a warning and take care of it before >> merging into master. As an example, if you deprecate a method, you will >> have to check where it is used, where it can be updated to its successor >> and where it's required to use the deprecated method and appropriately >> put macros there. >> IMO this workflow should be preferred to merging code which produces >> warnings of which the community (i.e. "somebody out there") will have to >> take care of. > Big +1 from me to this! ... this is now merged and we now have an excellent signal/noise ratio in the compiler warning pane. Cheers signature.asc Description: OpenPGP digital signature ___ Qgis-developer mailing list Qgis-developer@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [Qgis-developer] Q/A Compiler Warnings
On 16 June 2015 at 21:54, Matthias Kuhn wrote: > In the quest for an always closer-to-perfection state of code, some > goblins tried to get the warning count (on gcc and clang) yesterday as > close to zero as possible. This was done in order to allow you (as a > developer) to see if your code changes cause warnings without scrolling > through heaps of meaningless warnings. Thanks goblins! Your work in much appreciated :) > > At the moment most of the warnings left originate from the geometry > refactoring and point to empty methods which have not yet been > implemented. These warnings point to real problems and should be fixed > as soon as possible, in order to have some time left before release to > be able to test the fixes before the release. @Marco, what is your > schedule concerning these? I've just pushed some fixes to master which reimplement these stubbed methods and comment out some of the unimplemented new methods. 2.10 release was approaching too close for comfort to leave these in! @Marco Please let me know if I've made any errors doing this... > The plan is to let travis test for warnings and if there are warnings > mark the build red. With this in place, an author of a commit will be > required to spend some thoughts on a warning and take care of it before > merging into master. As an example, if you deprecate a method, you will > have to check where it is used, where it can be updated to its successor > and where it's required to use the deprecated method and appropriately > put macros there. > IMO this workflow should be preferred to merging code which produces > warnings of which the community (i.e. "somebody out there") will have to > take care of. Big +1 from me to this! Nyall ___ Qgis-developer mailing list Qgis-developer@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [Qgis-developer] Q/A Compiler Warnings
On Tue, Jun 16, 2015 at 02:27:10PM +0200, Matthias Kuhn wrote: > In an ideal world, all internal code would be ported to the new API and > some tests guarantee that the legacy API keeps working as expected. > Sometimes it may also be ok to leave internal code use the deprecated > API. It just has to happen consciously. > See here e.g. > https://github.com/qgis/QGIS/blob/master/tests/src/core/testqgsmaptopixel.cpp#L34-L36 Thanks, perfect example of the ideal world :) --strk; ___ Qgis-developer mailing list Qgis-developer@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [Qgis-developer] Q/A Compiler Warnings
On 06/16/2015 02:03 PM, Sandro Santilli wrote: > On Tue, Jun 16, 2015 at 01:54:34PM +0200, Matthias Kuhn wrote: > >> The plan is to let travis test for warnings and if there are warnings >> mark the build red. With this in place, an author of a commit will be >> required to spend some thoughts on a warning and take care of it before >> merging into master. As an example, if you deprecate a method, you will >> have to check where it is used, where it can be updated to its successor >> and where it's required to use the deprecated method and appropriately >> put macros there. > I'm happy to fight the warnings, but in this final plan I envision the risk > to break backward compatibility. What I mean is that if someone deprecates > a method, he should still make sure the deprecated method still works. > Having some internal code path using the deprecated method (maybe only in > tests) would serve that purpose. But will raise the warnings. In an ideal world, all internal code would be ported to the new API and some tests guarantee that the legacy API keeps working as expected. Sometimes it may also be ok to leave internal code use the deprecated API. It just has to happen consciously. > > Incidentally, you hadn't mentioned how your "hushing" macro work, as they > may be an answer to this concern of mine. See here e.g. https://github.com/qgis/QGIS/blob/master/tests/src/core/testqgsmaptopixel.cpp#L34-L36 ___ Qgis-developer mailing list Qgis-developer@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [Qgis-developer] Q/A Compiler Warnings
On Tue, Jun 16, 2015 at 01:54:34PM +0200, Matthias Kuhn wrote: > The plan is to let travis test for warnings and if there are warnings > mark the build red. With this in place, an author of a commit will be > required to spend some thoughts on a warning and take care of it before > merging into master. As an example, if you deprecate a method, you will > have to check where it is used, where it can be updated to its successor > and where it's required to use the deprecated method and appropriately > put macros there. I'm happy to fight the warnings, but in this final plan I envision the risk to break backward compatibility. What I mean is that if someone deprecates a method, he should still make sure the deprecated method still works. Having some internal code path using the deprecated method (maybe only in tests) would serve that purpose. But will raise the warnings. Incidentally, you hadn't mentioned how your "hushing" macro work, as they may be an answer to this concern of mine. --strk; ___ Qgis-developer mailing list Qgis-developer@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/qgis-developer
[Qgis-developer] Q/A Compiler Warnings
In the quest for an always closer-to-perfection state of code, some goblins tried to get the warning count (on gcc and clang) yesterday as close to zero as possible. This was done in order to allow you (as a developer) to see if your code changes cause warnings without scrolling through heaps of meaningless warnings. **What was done?** Mostly taking care of deprecated code QgsMapToPixel which could mostly be ported to the new methods including rotation. Some leftovers were being made quiet with macros. Having somebody take care of these would be appreciated. QgsFeature::geometryAndOwnership() is deprecated because since QgsGeometry is implicitly shared, a copy of the geometry is inexpensive. Hence this easy-to-get-wrong (double-delete, memory leaks) method has been deprecated. It does not hurt to leave it in place where it is being used now and in feature freeze the goblins did not want to risk breaking things there so they used the macro as well to make them quiet. Some other warnings coming from documentation and data types have been fixed as well. **What's left to do?** At the moment most of the warnings left originate from the geometry refactoring and point to empty methods which have not yet been implemented. These warnings point to real problems and should be fixed as soon as possible, in order to have some time left before release to be able to test the fixes before the release. @Marco, what is your schedule concerning these? Then there will be only one warning left about a double-translations, I am pretty sure that we can handle this. **What's the plan for the future?** The plan is to let travis test for warnings and if there are warnings mark the build red. With this in place, an author of a commit will be required to spend some thoughts on a warning and take care of it before merging into master. As an example, if you deprecate a method, you will have to check where it is used, where it can be updated to its successor and where it's required to use the deprecated method and appropriately put macros there. IMO this workflow should be preferred to merging code which produces warnings of which the community (i.e. "somebody out there") will have to take care of. Cheers, Matthias signature.asc Description: OpenPGP digital signature ___ Qgis-developer mailing list Qgis-developer@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/qgis-developer
