Smart Pointers

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Smart Pointers

Robert Middleton
So I've started work on converting everything over to smart pointers.  You can check it out on my github here: https://github.com/rm5248/log4cxx-testing/tree/smart_pointers

Some things to note so far:

  • This uses a quick and dirty mechanism in autotools to switch between C++11 and boost.  This is where the question of autotools vs cmake vs (something else) is important, as each build system has their own mechanism for finding and verifying that what the user wants to compile with is found.
  • I purged ObjectImpl completely.  This was the class that was(attempting) to provide some level of smart pointers, but it was really reference counting.  You had to manually do addRef/releaseRef on each object.  I didn't delve too deeply into the code, but it seems like this could cause memory leaks if the references were not correctly handled.
  • There are a lot of methods with ObjectPtr& parameters.  This is bad for smart pointers, since the internal refcount of the pointer won't get incremented(you can check the current refcount with the use_count() method)
  • The macros for using smart pointers instead of the pre-existing way fixed most of the pointer problems.  If you're interested, the relevant lines of code are here: https://github.com/rm5248/log4cxx-testing/blob/9ed12cc2187fd81b7d75dfb79abaffb9cc1ce169/src/main/include/log4cxx/log4cxx.h.in#L47
  • Some of the method signatures have to change.  Because of the way smart pointers work, you can't return a polymorphic object from a method.  For example, some methods were returning ObjectPtr, but changing this to shared_ptr<Object> doesn't work.  See this stackoverflow question for some more information: http://stackoverflow.com/questions/196733/how-can-i-use-covariant-return-types-with-smart-pointers
  • Possible solution to the above problem: pas a ** pointer to a function which returns a pointer to the newly created object in this parameter instead of the return(SQLite does this when opening a new database)
  • Because of the above, I had to add a few dynamic_casts into the code to get it to compile(as of right now, it doesn't fully compile).  A better solution here is to refactor the code to not require this, since either way this is a questionable design.  The current code assumes that objects have an instanceof() method; why this was used instead of dynamic_cast I don't know, but I suspect that the reason is to avoid using RTTI information in the first place, since that is (relatively) slow.
  • On a lot of the files, I also haven't looked in-depth at what exactly they are doing, so some of the fixes that I am doing are educated guesses as to what I think it is trying to do.
Overall, everything is going somewhat smoothly.  Also note that the changes I am doing probably break API compatability, but I'm not sure how bad it will be at the moment.  Given that ObjectPtr overrode operator-> and shared_ptr does as well, it seems that most of this will be transparent to other code(this is why I was able to pretty quickly change the LOG4CXX_PTR_DEF macro and have things almost work).

-Robert Middleton
Reply | Threaded
Open this post in threaded view
|

Re: Smart Pointers

Robert Middleton


On Sun, Nov 20, 2016 at 9:39 AM, Thorsten Schöning <[hidden email]> wrote:
Guten Tag Robert Middleton,
am Sonntag, 20. November 2016 um 03:31 schrieben Sie:

> So I've started work on converting everything over to smart
> pointers.  You can check it out on my github here:
> https://github.com/rm5248/log4cxx-testing/tree/smart_pointers

Great work. Any tips on how I could easily merge that with a separate
SVN branch? Especially if you keep committing. I guess I'll try
something using the SVN bridge of GitHub...

It depends on how you want to merge it in.  If you don't care about the history, just do a 'git diff' to whatever the first commit was, which will essentially squash everything into one diff that you can apply via patch.
 

> This uses a quick and dirty mechanism in autotools to switch
> between C++11 and boost.[...]

You support autotools and one can easily provide the macros
independently, doesn't look that "dirty" to me.

The only thing that it doesn't do is run checks to make sure that either the compiler supports C++11, or if boost is installed; that's the dirty part of it.  Ideally it should check to see what is installed and fail if nothing is found in the configure phase.
 

> The
> current code assumes that objects have an instanceof() method; why
> this was used instead of dynamic_cast I don't know, but I suspect
> that the reason is to avoid using RTTI information in the first
> place, since that is (relatively) slow.

My understanding of the codebase is that it was converted mainly
directly from Java and you have instanceof there.

Mit freundlichen Grüßen,

Thorsten Schöning

--
Thorsten Schöning       E-Mail: [hidden email]
AM-SoFT IT-Systeme      http://www.AM-SoFT.de/

Telefon...........05151-  9468- 55
Fax...............05151-  9468- 88
Mobil..............0178-8 9468- 04

AM-SoFT GmbH IT-Systeme, Brandenburger Str. 7c, 31789 Hameln
AG Hannover HRB 207 694 - Geschäftsführer: Andreas Muchow


Reply | Threaded
Open this post in threaded view
|

Re: Smart Pointers

Robert Middleton
Success!  log4cxx now compiles, but it almost certainly doesn't work yet.  I need to do some testing on it.

Things that need to be changed:

  • Figure out what classes/methods used to take an ObjectPtr&.  If they should actually own this object in any way, change this to not pass by reference so that the smart pointer refcount actually gets incremented.  It seems that log4cxx works as long as objects don't get deconstructed.  Since the internal pointer refcount of shared_ptr doesn't get incremented in this case, the method will be called, the smart pointer goes out of scope in the calling method, and then I expect that the process will get a SIGSEGV.  
  • Some methods have been changed to pass a raw pointer to prevent the above; depending on what their purposes is they could perhaps pass a weak_ptr instead.  The advatage of passing shared_ptr& instead is that it is faster code, since the pointer doesn't have to lock a mutex and increment variables.
  • Some methods take in a reference instead of a pointer even if they modify the parameter.  Generally, the convention is to pass in a pointer in this case; Google's style guide(https://google.github.io/styleguide/cppguide.html#Reference_Arguments) states this as well.  So I would recommend that we do this as well.
Other notes:
  • Some of the publicly-visible APIs did change.  So a major version bump would be appropriate.
  • The indent style is rather inconsistent to say the least.  I made an astyle config for it and applied it to all files; it's a separate commit since it touches everything.
  • I want to run the code through cppcheck to help find bugs.
  • In addition to cppcheck, adding warnings for GCC at least can help find possible bugs in the code.  
Flags that the Linux kernel uses(note C not C++, but most of these are still applicable).  This is not an exhaustive list and is a subset of -Wall, since -Wall can produce a lot of output that is not always helpful.
This is from the main Makefile.
KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
		   -fno-strict-aliasing -fno-common \
		   -Werror-implicit-function-declaration \
		   -Wno-format-security \
		   -std=gnu8
KBUILD_CFLAGS	+= $(call cc-option,-fno-delete-null-pointer-checks,)
KBUILD_CFLAGS	+= $(call cc-disable-warning,maybe-uninitialized,)
KBUILD_CFLAGS   += $(call cc-option,-Werror=strict-prototypes)

# Prohibit date/time macros, which would make the build non-deterministic
KBUILD_CFLAGS   += $(call cc-option,-Werror=date-time)

# enforce correct pointer usage
KBUILD_CFLAGS   += $(call cc-option,-Werror=incompatible-pointer-types)

If anybody wants to try it out, the commit that it works on(without style changes) is this one(I accidentally committed binaries, so this commit simply removes them): https://github.com/rm5248/log4cxx-testing/commit/10f8fdd99bde2410ee92fefb46a414a5bf12669a

Otherwise, the commit with all of the formatting changes is here: https://github.com/rm5248/log4cxx-testing/commit/6c5e3f3ba7075fbe7d82001e90510e4ed9c19029

-Robert Middleton

On Sun, Nov 20, 2016 at 12:10 PM, Robert Middleton <[hidden email]> wrote:


On Sun, Nov 20, 2016 at 9:39 AM, Thorsten Schöning <[hidden email]> wrote:
Guten Tag Robert Middleton,
am Sonntag, 20. November 2016 um 03:31 schrieben Sie:

> So I've started work on converting everything over to smart
> pointers.  You can check it out on my github here:
> https://github.com/rm5248/log4cxx-testing/tree/smart_pointers

Great work. Any tips on how I could easily merge that with a separate
SVN branch? Especially if you keep committing. I guess I'll try
something using the SVN bridge of GitHub...

It depends on how you want to merge it in.  If you don't care about the history, just do a 'git diff' to whatever the first commit was, which will essentially squash everything into one diff that you can apply via patch.
 

> This uses a quick and dirty mechanism in autotools to switch
> between C++11 and boost.[...]

You support autotools and one can easily provide the macros
independently, doesn't look that "dirty" to me.

The only thing that it doesn't do is run checks to make sure that either the compiler supports C++11, or if boost is installed; that's the dirty part of it.  Ideally it should check to see what is installed and fail if nothing is found in the configure phase.
 

> The
> current code assumes that objects have an instanceof() method; why
> this was used instead of dynamic_cast I don't know, but I suspect
> that the reason is to avoid using RTTI information in the first
> place, since that is (relatively) slow.

My understanding of the codebase is that it was converted mainly
directly from Java and you have instanceof there.

Mit freundlichen Grüßen,

Thorsten Schöning

--
Thorsten Schöning       E-Mail: [hidden email]
AM-SoFT IT-Systeme      http://www.AM-SoFT.de/

Telefon...........05151-  9468- 55
Fax...............05151-  9468- 88
Mobil..............0178-8 9468- 04

AM-SoFT GmbH IT-Systeme, Brandenburger Str. 7c, 31789 Hameln
AG Hannover HRB 207 694 - Geschäftsführer: Andreas Muchow



Reply | Threaded
Open this post in threaded view
|

Re: Smart Pointers

Robert Middleton
I'm not sure what the status is of log4cxx at the moment, but progress has been made on my branch since the last time I posted.  I have fixed all of the problems that the unit tests found once I converted everything over to smart pointers.  Fortunately, there are a large number of unit tests that cover all aspects of the code, so the large majority of issues should have been fixed as of this point.  This still requires testing on actual user programs though.


-Robert Middleton

On Wed, Nov 23, 2016 at 6:43 PM, Robert Middleton <[hidden email]> wrote:
Success!  log4cxx now compiles, but it almost certainly doesn't work yet.  I need to do some testing on it.

Things that need to be changed:

  • Figure out what classes/methods used to take an ObjectPtr&.  If they should actually own this object in any way, change this to not pass by reference so that the smart pointer refcount actually gets incremented.  It seems that log4cxx works as long as objects don't get deconstructed.  Since the internal pointer refcount of shared_ptr doesn't get incremented in this case, the method will be called, the smart pointer goes out of scope in the calling method, and then I expect that the process will get a SIGSEGV.  
  • Some methods have been changed to pass a raw pointer to prevent the above; depending on what their purposes is they could perhaps pass a weak_ptr instead.  The advatage of passing shared_ptr& instead is that it is faster code, since the pointer doesn't have to lock a mutex and increment variables.
  • Some methods take in a reference instead of a pointer even if they modify the parameter.  Generally, the convention is to pass in a pointer in this case; Google's style guide(https://google.github.io/styleguide/cppguide.html#Reference_Arguments) states this as well.  So I would recommend that we do this as well.
Other notes:
  • Some of the publicly-visible APIs did change.  So a major version bump would be appropriate.
  • The indent style is rather inconsistent to say the least.  I made an astyle config for it and applied it to all files; it's a separate commit since it touches everything.
  • I want to run the code through cppcheck to help find bugs.
  • In addition to cppcheck, adding warnings for GCC at least can help find possible bugs in the code.  
Flags that the Linux kernel uses(note C not C++, but most of these are still applicable).  This is not an exhaustive list and is a subset of -Wall, since -Wall can produce a lot of output that is not always helpful.
This is from the main Makefile.
KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
		   -fno-strict-aliasing -fno-common \
		   -Werror-implicit-function-declaration \
		   -Wno-format-security \
		   -std=gnu8
KBUILD_CFLAGS	+= $(call cc-option,-fno-delete-null-pointer-checks,)
KBUILD_CFLAGS	+= $(call cc-disable-warning,maybe-uninitialized,)
KBUILD_CFLAGS   += $(call cc-option,-Werror=strict-prototypes)

# Prohibit date/time macros, which would make the build non-deterministic
KBUILD_CFLAGS   += $(call cc-option,-Werror=date-time)

# enforce correct pointer usage
KBUILD_CFLAGS   += $(call cc-option,-Werror=incompatible-pointer-types)

If anybody wants to try it out, the commit that it works on(without style changes) is this one(I accidentally committed binaries, so this commit simply removes them): https://github.com/rm5248/log4cxx-testing/commit/10f8fdd99bde2410ee92fefb46a414a5bf12669a

Otherwise, the commit with all of the formatting changes is here: https://github.com/rm5248/log4cxx-testing/commit/6c5e3f3ba7075fbe7d82001e90510e4ed9c19029

-Robert Middleton

On Sun, Nov 20, 2016 at 12:10 PM, Robert Middleton <[hidden email]> wrote:


On Sun, Nov 20, 2016 at 9:39 AM, Thorsten Schöning <[hidden email]> wrote:
Guten Tag Robert Middleton,
am Sonntag, 20. November 2016 um 03:31 schrieben Sie:

> So I've started work on converting everything over to smart
> pointers.  You can check it out on my github here:
> https://github.com/rm5248/log4cxx-testing/tree/smart_pointers

Great work. Any tips on how I could easily merge that with a separate
SVN branch? Especially if you keep committing. I guess I'll try
something using the SVN bridge of GitHub...

It depends on how you want to merge it in.  If you don't care about the history, just do a 'git diff' to whatever the first commit was, which will essentially squash everything into one diff that you can apply via patch.
 

> This uses a quick and dirty mechanism in autotools to switch
> between C++11 and boost.[...]

You support autotools and one can easily provide the macros
independently, doesn't look that "dirty" to me.

The only thing that it doesn't do is run checks to make sure that either the compiler supports C++11, or if boost is installed; that's the dirty part of it.  Ideally it should check to see what is installed and fail if nothing is found in the configure phase.
 

> The
> current code assumes that objects have an instanceof() method; why
> this was used instead of dynamic_cast I don't know, but I suspect
> that the reason is to avoid using RTTI information in the first
> place, since that is (relatively) slow.

My understanding of the codebase is that it was converted mainly
directly from Java and you have instanceof there.

Mit freundlichen Grüßen,

Thorsten Schöning

--
Thorsten Schöning       E-Mail: [hidden email]
AM-SoFT IT-Systeme      http://www.AM-SoFT.de/

Telefon...........05151-  9468- 55
Fax...............05151-  9468- 88
Mobil..............0178-8 9468- 04

AM-SoFT GmbH IT-Systeme, Brandenburger Str. 7c, 31789 Hameln
AG Hannover HRB 207 694 - Geschäftsführer: Andreas Muchow




Reply | Threaded
Open this post in threaded view
|

Re: Smart Pointers

Robert Middleton
I can make a new fork and do the PR.  There are still a few things
that I need to check though; I think I figured out why the original
code had the special log4cxx casting macros.  Apparently the
dynamic_cast doesn't always work across shared library boundaries, so
that could cause a problem.  Some of the code is also a little
inconsistent at the moment and should be more standardized(I changed
the casting about halfway through).

I would like to do this after an actual release though, as since it
touches pretty much everything I wouldn't want to break code for
anybody who is using a current SVN version, since there are a lot of
fixes for current bugs in SVN(well, git now).

-Robert Middleton

On Tue, Apr 11, 2017 at 8:24 AM, Thorsten Schöning
<[hidden email]> wrote:

> Guten Tag Robert Middleton,
> am Freitag, 20. Januar 2017 um 03:41 schrieben Sie:
>
>> git link:
>> https://github.com/rm5248/log4cxx-testing/tree/smart_pointers
>
> Now that we have a GIT repo, how do we deal with your branch? Are you
> able to fork the new GitHub mirror and apply your changes easily again
> in the fork? From my understanding you are only afterwards able to
> create a PR on GitHub.
>
> Alternatively I need to add your repo as a remote in my repo and
> merge directly I guess?
>
> I think a somewhat "official" PR from a fork of you against the new
> mirror would be cleanest solution.
>
> Mit freundlichen Grüßen,
>
> Thorsten Schöning
>
> --
> Thorsten Schöning       E-Mail: [hidden email]
> AM-SoFT IT-Systeme      http://www.AM-SoFT.de/
>
> Telefon...........05151-  9468- 55
> Fax...............05151-  9468- 88
> Mobil..............0178-8 9468- 04
>
> AM-SoFT GmbH IT-Systeme, Brandenburger Str. 7c, 31789 Hameln
> AG Hannover HRB 207 694 - Geschäftsführer: Andreas Muchow
>