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
   <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 <osfan6...@gmail.com>
wrote:

>
>
> On Sun, Nov 20, 2016 at 9:39 AM, Thorsten Schöning <tschoen...@am-soft.de>
> 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: thorsten.schoen...@am-soft.de
>> 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 via email to