Re: [cmake-developers] Proposal: Using smart pointers to own dynamically allocated memory
I will do that. It's also easier for me to track the changes that way. Regards, Tushar On Sat, Sep 14, 2019 at 12:04 AM Brad King wrote: > > On 9/13/19 1:30 PM, Tushar Maheshwari wrote: > > Thanks for the quick response. > > I have my commits separated by file groups. I'll open small MRs > > collecting the related groups. > > Thanks. Please just keep a couple MRs open at a time so we don't > overwhelm the CI builders. After some are merged you can open more. > > -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: https://cmake.org/mailman/listinfo/cmake-developers
Re: [cmake-developers] Proposal: Using smart pointers to own dynamically allocated memory
On 9/13/19 1:30 PM, Tushar Maheshwari wrote: > Thanks for the quick response. > I have my commits separated by file groups. I'll open small MRs > collecting the related groups. Thanks. Please just keep a couple MRs open at a time so we don't overwhelm the CI builders. After some are merged you can open more. -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: https://cmake.org/mailman/listinfo/cmake-developers
Re: [cmake-developers] Proposal: Using smart pointers to own dynamically allocated memory
Thanks for the quick response. I have my commits separated by file groups. I'll open small MRs collecting the related groups. Please comment if the grouping is incorrect or if a commit/diff needs to be moved to a different branch. Thanks, Tushar On Fri, Sep 13, 2019 at 10:46 PM Brad King wrote: > > On 9/13/19 12:58 PM, Kyle Edwards via cmake-developers wrote: > > On Fri, 2019-09-13 at 22:08 +0530, Tushar Maheshwari wrote: > >> I have pushed some sample commits to > >> https://gitlab.kitware.com/tusharpm/cmake/commits/smart_mem. > >> > >> If this is something I can pursue, I would appreciate a review of my > >> changes to better suit the project. > > > > We have already made lots of progress in replacing manual delete's with > > std::unique_ptr's, and completing this modernization has been a goal of > > ours since we switched to C++11. I would very strongly encourage you to > > open a merge request with any progress you've made in this regard. > > Yes. Your branch changes a lot of areas and there are several other > rounds of refactoring going on so you may have trouble with conflicts. > I suggest grouping the changes (e.g. ExportSet) and opening a MR with > only one area. Once that is merged open a new MR for another area. > > Thanks, > -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: https://cmake.org/mailman/listinfo/cmake-developers
Re: [cmake-developers] Proposal: Using smart pointers to own dynamically allocated memory
On 9/13/19 12:58 PM, Kyle Edwards via cmake-developers wrote: > On Fri, 2019-09-13 at 22:08 +0530, Tushar Maheshwari wrote: >> I have pushed some sample commits to >> https://gitlab.kitware.com/tusharpm/cmake/commits/smart_mem. >> >> If this is something I can pursue, I would appreciate a review of my >> changes to better suit the project. > > We have already made lots of progress in replacing manual delete's with > std::unique_ptr's, and completing this modernization has been a goal of > ours since we switched to C++11. I would very strongly encourage you to > open a merge request with any progress you've made in this regard. Yes. Your branch changes a lot of areas and there are several other rounds of refactoring going on so you may have trouble with conflicts. I suggest grouping the changes (e.g. ExportSet) and opening a MR with only one area. Once that is merged open a new MR for another area. Thanks, -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: https://cmake.org/mailman/listinfo/cmake-developers
Re: [cmake-developers] Proposal: Using smart pointers to own dynamically allocated memory
On Fri, 2019-09-13 at 22:08 +0530, Tushar Maheshwari wrote: > Hi, > > I am a C++ developer and a modern-C++ enthusiast. In the interest of > modernizing the codebase, I propose using smart pointers to handle > dynamically allocated memory. I would like to get the feedback from > the developers if this change is planned/attempted/desirable or > inapplicable for CMake. > > Currently, in the master branch (limiting to the Source directory): > - delete expression appears 127 times (2 of those are false > positives). > $ grep -Er 'delete(\[\])? \S+;' Source | wc -l > 127 > - cmDeleteAll function (a helper to delete a Range of objects) is > used 40 times. > $ grep -Er 'cmDeleteAll\(.+\);' Source | wc -l > 40 > - [Skipping the `free` stats. I can investigate that if required.] > > Many of these are great candidates for `std::unique_ptr`. This can > reduce the destructors of many classes to default, and in some cases > achieve the rule of zero. This aligns well with the "Resource > management" section of the CppCoreGuidelines. > However, it might be impractical to replace some occurrences, like > the > interfaces with C libraries. > > I have pushed some sample commits to > https://gitlab.kitware.com/tusharpm/cmake/commits/smart_mem. > > One drawback I noticed while changing some members to use smart > pointers is the boilerplate required to expose them to the callers. > There might be a cleaner way than passing references to smart > pointers. I would like to discuss those options also, if possible. > > If this is something I can pursue, I would appreciate a review of my > changes to better suit the project. We have already made lots of progress in replacing manual delete's with std::unique_ptr's, and completing this modernization has been a goal of ours since we switched to C++11. I would very strongly encourage you to open a merge request with any progress you've made in this regard. Kyle -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: https://cmake.org/mailman/listinfo/cmake-developers
[cmake-developers] Proposal: Using smart pointers to own dynamically allocated memory
Hi, I am a C++ developer and a modern-C++ enthusiast. In the interest of modernizing the codebase, I propose using smart pointers to handle dynamically allocated memory. I would like to get the feedback from the developers if this change is planned/attempted/desirable or inapplicable for CMake. Currently, in the master branch (limiting to the Source directory): - delete expression appears 127 times (2 of those are false positives). $ grep -Er 'delete(\[\])? \S+;' Source | wc -l 127 - cmDeleteAll function (a helper to delete a Range of objects) is used 40 times. $ grep -Er 'cmDeleteAll\(.+\);' Source | wc -l 40 - [Skipping the `free` stats. I can investigate that if required.] Many of these are great candidates for `std::unique_ptr`. This can reduce the destructors of many classes to default, and in some cases achieve the rule of zero. This aligns well with the "Resource management" section of the CppCoreGuidelines. However, it might be impractical to replace some occurrences, like the interfaces with C libraries. I have pushed some sample commits to https://gitlab.kitware.com/tusharpm/cmake/commits/smart_mem. One drawback I noticed while changing some members to use smart pointers is the boilerplate required to expose them to the callers. There might be a cleaner way than passing references to smart pointers. I would like to discuss those options also, if possible. If this is something I can pursue, I would appreciate a review of my changes to better suit the project. Thanks, Tushar -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: https://cmake.org/mailman/listinfo/cmake-developers
