Use Shared pointer instead of raw pointer #109
Conversation
…aw pointer This will return a managed pointer to the user instead of an unmanaged pointer. This helps static code analyzers from flagged false positive memory leaks It is becoming harder and harder for projects to use this project because some customers require a static code analyzer. By returning a managed pointer, those projects can continue to use this amazing library.
| @@ -6,6 +6,7 @@ | |||
| // This file defines template functions that are declared in | |||
| // ../value_semantic.hpp. | |||
|
|
|||
| #include <boost/smart_ptr/make_shared.hpp> | |||
There was a problem hiding this comment.
This is a downside to using the shared pointers instead of raw pointers. If you include program_options, it will include make_shared (and later on in this pull request enable_shared_from_this)
| @@ -167,6 +167,15 @@ namespace program_options { | |||
| operator()(const char* name, | |||
| const char* description); | |||
|
|
|||
| options_description_easy_init& | |||
| operator()(const char* name, | |||
| shared_ptr<const value_semantic> s); | |||
There was a problem hiding this comment.
I left the functions that used const value_semantic* s for backwards compatibility. The last thing I want to do is break someone's code that depends on it.
| @@ -315,6 +316,20 @@ void test_value_name() | |||
| ); | |||
| } | |||
|
|
|||
| void test_backwards_compatibility_with_raw_pointer() | |||
There was a problem hiding this comment.
I thought it was a good idea to add a test case to test the backwards compatibility.
…version from 'int' to non-scalar type
|
Looking at the checks, I see that four of them are failing, but I don't know why. It looks like the first two are failing because it is failing to download something. The second two look like they are failing because putenv is not declared. Looking at past pulls that have been merged, they also have the same findings, which makes me think I didn't break them in this pull. I was going to try and fix them but I read in the boost documentation to avoid drive by fixes... so I held off on that for now... |
|
At this point, the only other static code analysis tool I have been able to use is cppcheck, and it does not have an issue with this because it doesn't look into the header files that you include. I have been unable to get another static code analyzer yet. I am curious though what your thoughts are on this change? |
Update value_semantic to return a boost shared pointer instead of a raw pointer
This will return a managed pointer to the user instead of an unmanaged pointer.
This helps static code analyzers from flagged false positive memory leaks
It is becoming harder for projects to use program_options because some customers require certain static code analyzers. By returning a managed pointer, those projects can continue to use this amazing library. The certain static code analyzer that we are required to use flags this. Because the pointer is being declared in a hpp and being stored into a shared pointer in a cpp, static code analyzers wouldn't be able to see it. I have at least one more static code analyzer results coming (may end up getting two more for a total of three) to see if those static code analyzers also flag this.
I agree that the code with raw pointers is correct, and that we shouldn't degrade our code so that static code analyzers can understand them. I took a stab at trying to update the library to return a shared pointer instead of a raw pointer, and I don't feel like it has made the code worst. I figure at the very least if you don't want this change, you could tell me why using the shared pointers is a bad idea. I also figure that other projects are running into this.
I am sorry if this pull_request is very sloppy. This is my first time making a pull to an open source project... But I hope this to be a good learning experience so that one day I can actually help open source projects.