You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by Robert Middleton <rm...@apache.org> on 2021/11/06 01:57:03 UTC
[log4cxx] ABI stability
I've been working on making log4cxx ABI stable, and I believe that I
have converted most(if not all) classes at this point to be stable.
I've got a branch here that has all of the changes:
https://github.com/apache/logging-log4cxx/tree/LOGCXX-510
There are some notes on how this is done here:
https://github.com/apache/logging-log4cxx/blob/LOGCXX-510/src/site/markdown/library-design.md
If people can provide feedback on how this currently looks and if they
notice anything wrong, I'd greatly appreciate it. I'm not planning on
merging this to master for a while; since this contains a number of
breaking changes I want to keep it separate(and do some other breaking
changes in the near future).
-Robert Middleton
Re: [log4cxx] ABI stability
Posted by Thorsten Schöning <ts...@am-soft.de>.
Guten Tag Robert Middleton,
am Montag, 29. November 2021 um 04:53 schrieben Sie:
> At this point, I believe that I have converted all of the important
> classes to be ABI-stable.[...]
Not that it's too representative, but things properly build and test
for my pretty current VS 17.02 as well.
> As per the LocationInfo, in the future we should probably make more
> use of std::string_view(C++17) or the equivalent boost::string_view.
+1
> I'll merge this into a new branch(log4cxx-next?) shortly[...]
Not too important, but as there's already some "latest_stable",
"next_stable" might make sense as well. "latest|next|old_stable" has
been used for the old website.
> * Remove any APR references in the header files.
+1. The less dependencies the better and preferring BOOST over APR
seems to make sense for some C++ app.
Mit freundlichen Grüßen
Thorsten Schöning
--
AM-SoFT IT-Service - Bitstore Hameln GmbH
Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK
E-Mail: Thorsten.Schoening@AM-SoFT.de
Web: http://www.AM-SoFT.de/
Tel: 05151- 9468- 0
Tel: 05151- 9468-55
Fax: 05151- 9468-88
Mobil: 0178-8 9468-04
AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln
AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska
Re: [log4cxx] ABI stability
Posted by Robert Middleton <rm...@apache.org>.
At this point, I believe that I have converted all of the important
classes to be ABI-stable. There are a few that I have missed, some
probably unintentional, some because I plan on removing them. There
are also a handful of classes that don't need to be(nor perhaps should
be) ABI-stable - I'm thinking mostly of the LocationInfo in PR#75. We
should probably allocate a few extra bytes onto the end of any classes
like LocationInfo incase we ever need to add extra members(the current
PR does break the ABI as it changes the size of the LocationInfo class
- we don't make any claims as to being ABI-stable at the moment, so it
doesn't really matter).
As per the LocationInfo, in the future we should probably make more
use of std::string_view(C++17) or the equivalent boost::string_view.
The string_view would be perfect for use in the LocationInfo class,
since it just needs to point at a const char* but having the ability
to do string-like functions would be useful. I suspect that there are
several other places where we can make use of string_view as well,
perhaps gaining some performance improvements.
Some notes on string_view: https://rules.sonarsource.com/cpp/RSPEC-6009
I'll merge this into a new branch(log4cxx-next?) shortly, unless
anybody finds any problems. Once that is complete, it should be much
easier to fix problems going forward. I think my immediate plans
after this would be to:
* Get the multi-process RollingFileAppender working. Since you
currently need a custom-built version of log4cxx to make it work, it's
of somewhat limited usefulness at the moment(I don't think there are
any tests at the moment either, so that's not too useful)
* Remove any APR references in the header files.
Since most of what we use APR for is in C++11, my plan is to try to
eliminate APR for basic logging setups. e.g. the date/time that is
used can be replaced with std::chrono, threads we have already
switched to std::thread, networking we can switch between using APR
and using boost::asio, etc. This /should/ mean that if you have a
C++11 only compiler, you'll need to have boost installed to build, but
if you have a C++17 compiler you won't need any dependencies to build.
-Robert Middleton
On Sun, Nov 7, 2021 at 12:02 PM Thorsten Schöning <ts...@am-soft.de> wrote:
>
> Guten Tag Robert Middleton,
> am Sonntag, 7. November 2021 um 17:36 schrieben Sie:
>
> > I don't see any advantage to this over a macro, unless you were
> > thinking of something else?
>
> That's exactly what I meant and the benefits I see are simply avoiding
> a macro and having something documented for the class as part of its
> (private) API. With having that method abstract, one can even force
> it's implementation in subclasses, resulting in a more uniform access
> to the data across the code base etc.
>
> That's different with a macro, which might result in bad error
> messages during compilation, might be named differently across
> classes, might not be available because devs missed it etc.
>
> Though, it's a matter of taste most likely.
>
> Mit freundlichen Grüßen
>
> Thorsten Schöning
>
> --
> AM-SoFT IT-Service - Bitstore Hameln GmbH
> Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK
>
> E-Mail: Thorsten.Schoening@AM-SoFT.de
> Web: http://www.AM-SoFT.de/
>
> Tel: 05151- 9468- 0
> Tel: 05151- 9468-55
> Fax: 05151- 9468-88
> Mobil: 0178-8 9468-04
>
> AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln
> AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska
>
>
>
>
Re: [log4cxx] ABI stability
Posted by Thorsten Schöning <ts...@am-soft.de>.
Guten Tag Robert Middleton,
am Sonntag, 7. November 2021 um 17:36 schrieben Sie:
> I don't see any advantage to this over a macro, unless you were
> thinking of something else?
That's exactly what I meant and the benefits I see are simply avoiding
a macro and having something documented for the class as part of its
(private) API. With having that method abstract, one can even force
it's implementation in subclasses, resulting in a more uniform access
to the data across the code base etc.
That's different with a macro, which might result in bad error
messages during compilation, might be named differently across
classes, might not be available because devs missed it etc.
Though, it's a matter of taste most likely.
Mit freundlichen Grüßen
Thorsten Schöning
--
AM-SoFT IT-Service - Bitstore Hameln GmbH
Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK
E-Mail: Thorsten.Schoening@AM-SoFT.de
Web: http://www.AM-SoFT.de/
Tel: 05151- 9468- 0
Tel: 05151- 9468-55
Fax: 05151- 9468-88
Mobil: 0178-8 9468-04
AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln
AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska
Re: [log4cxx] ABI stability
Posted by Robert Middleton <rm...@apache.org>.
I think I see what you're saying now. We can do something like this:
class A{
virtual APrivate* private_data(){
return priv.get();
}
unique_ptr<APrivate> priv;
};
class B: public A{
virtual BPrivate* private_data(){
return static_cast<BPrivate*>(priv.get());
}
};
I don't see any advantage to this over a macro, unless you were
thinking of something else?
-Robert Middleton
On Sat, Nov 6, 2021 at 12:42 PM Thorsten Schöning <ts...@am-soft.de> wrote:
>
> Guten Tag Robert Middleton,
> am Samstag, 6. November 2021 um 16:52 schrieben Sie:
>
> > Unfortunately not, for the reasons above. Since the parent class only
> > holds a single pointer to the private data, whatever data is stored
> > must be a subclass of the parent's private data. So we must cast it
> > to whatever class's private data that we are currently using.
>
> Understood, but does it really needs to be a macro? Why not use a
> method implementing the cast which can then be overridden by
> subclasses as necessary? Looks like a covariant return type to me,
> doesn't it?
>
> https://en.wikipedia.org/wiki/Covariant_return_type
>
> Mit freundlichen Grüßen
>
> Thorsten Schöning
>
> --
> AM-SoFT IT-Service - Bitstore Hameln GmbH
> Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK
>
> E-Mail: Thorsten.Schoening@AM-SoFT.de
> Web: http://www.AM-SoFT.de/
>
> Tel: 05151- 9468- 0
> Tel: 05151- 9468-55
> Fax: 05151- 9468-88
> Mobil: 0178-8 9468-04
>
> AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln
> AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska
>
>
>
>
Re: [log4cxx] ABI stability
Posted by Thorsten Schöning <ts...@am-soft.de>.
Guten Tag Robert Middleton,
am Samstag, 6. November 2021 um 16:52 schrieben Sie:
> Unfortunately not, for the reasons above. Since the parent class only
> holds a single pointer to the private data, whatever data is stored
> must be a subclass of the parent's private data. So we must cast it
> to whatever class's private data that we are currently using.
Understood, but does it really needs to be a macro? Why not use a
method implementing the cast which can then be overridden by
subclasses as necessary? Looks like a covariant return type to me,
doesn't it?
https://en.wikipedia.org/wiki/Covariant_return_type
Mit freundlichen Grüßen
Thorsten Schöning
--
AM-SoFT IT-Service - Bitstore Hameln GmbH
Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK
E-Mail: Thorsten.Schoening@AM-SoFT.de
Web: http://www.AM-SoFT.de/
Tel: 05151- 9468- 0
Tel: 05151- 9468-55
Fax: 05151- 9468-88
Mobil: 0178-8 9468-04
AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln
AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska
Re: [log4cxx] ABI stability
Posted by Robert Middleton <os...@gmail.com>.
> Great work! Is there something I should specifically look out when
> testing or would it be fione to know if things run at all already?
>
Mostly just looking for anything obvious that I may have missed, or
concerns with the implementation.
> Get a lot of warnings like this one currently:
>
> > Schweregrad Code Beschreibung Projekt Datei Zeile Unterdrückungszustand
> > Warnung C4251 "log4cxx::helpers::FileInputStream::m_priv": class "std::unique_ptr<log4cxx::helpers::FileInputStream::FileInputStreamPrivate,std::default_delete<log4cxx::helpers::FileInputStream::FileInputStreamPrivate>>" erfordert eine DLL-Schnittstelle, die von Clients von class "log4cxx::helpers::FileInputStream" verwendet wird C:\Users\tschoening\Documents\Svn\Src\Libs\trunk\C++\Logging\log4cxx\master\src\out\build\x64-Debug\src C:\Users\tschoening\Documents\Svn\Src\Libs\trunk\C++\Logging\log4cxx\master\src\src\main\include\log4cxx\helpers\fileinputstream.h 40
>
I'm going to guess that is something about not being able to use the
class across a DLL boundary. That's /probably/ not an issue for
reasons that I'll get into in a moment.
> Should this be the following instead? Looks like a copy&paste error.
>
> > std::unique_ptr<ParentPrivate> m_priv;
>
Yeah that's probably just a copy/paste error.
> Some naming discussion: How about using "ParentPriv" and
> "ParentPriv.h" instead of heaving "ParentPrivate", "Parent-private.h"
> and "m_priv". It's shorter, "-private" doesn't follow QT conventions
> anyway and the abbreviation "priv" is used already as well.
>
That sounds reasonable to me.
> When looking at your code, you are using some "XyPriv" and "xy_priv.h"
> and some "XyPrivate" and "xy_priv.h" already. For existing classes, I
> suggest keeping "xy_priv.h", but for newer classes "XyPriv" and
> "xyPriv.h" read better in my opinion. So I would suggest that in your
> new docs.
>
> About inheritance: Wouldn't it be better in most cases to provide
> protected or public getters in base classes anyway instead of
> accessing private data structs?
>
> I understand your example to show how it could be done in case no such
> getters are available or should be used for some reason. Or is that
> example showing how it needs to be done always regardless how the
> private data is accessed by subclasses?
>
The example shows how it needs to be done anyway. The reason for this
is explained a bit more in the Qt documentation, but here's a simple
explanation:
If we have a class with private data:
class A {
unique_ptr<APrivate> a_priv;
}
And we then need to subclass it:
class B : public A{
unique_ptr<BPrivate> b_priv;
}
Whenever we create class B, we now do two memory allocations(APrivate
and BPrivate). In the code for B, you then also need to keep track of
which superclass actually holds the data. If we instead subclass
'APrivate' in 'BPrivate', we can then store the pointer to 'BPrivate'
in the 'A' class.
struct APrivate{
int32_t a;
};
struct BPrivate : public APrivate{
int32_t b;
};
BPrivate is now 8 bytes in size. Because it is a subclass of
APrivate, we can store the pointer to BPrivate inside of A, thus
saving us a memory allocation and letting us pretend that we still
have a "normal" inheritance:
class A{
unique_ptr<APrivate> priv;
};
class B: public A{
B(){
priv = make_unique<BPriv>(); // 'priv' is of type APrivate, so
must downclass in B whenever we want to use it
}
};
> > #define priv static_cast<AndFilterPrivate*>(m_priv.get())
>
> Does it make sense to mention something like that in your new docs?
> Is it possible to make it a (generic?) getter or alike instead of a
> macro?
Unfortunately not, for the reasons above. Since the parent class only
holds a single pointer to the private data, whatever data is stored
must be a subclass of the parent's private data. So we must cast it
to whatever class's private data that we are currently using.
Because of this as well, the 'private' classes don't actually need to
be exported out of the library: you only need them if you are
subclassing an object and you are interested in doing only one memory
allocation for your private instance variables.
-Robert Middleton
Re: [log4cxx] ABI stability
Posted by Thorsten Schöning <ts...@am-soft.de>.
Guten Tag Robert Middleton,
am Samstag, 6. November 2021 um 02:57 schrieben Sie:
> There are some notes on how this is done here:
> https://github.com/apache/logging-log4cxx/blob/LOGCXX-510/src/site/markdown/library-design.md
Great work! Is there something I should specifically look out when
testing or would it be fione to know if things run at all already?
Get a lot of warnings like this one currently:
> Schweregrad Code Beschreibung Projekt Datei Zeile Unterdrückungszustand
> Warnung C4251 "log4cxx::helpers::FileInputStream::m_priv": class "std::unique_ptr<log4cxx::helpers::FileInputStream::FileInputStreamPrivate,std::default_delete<log4cxx::helpers::FileInputStream::FileInputStreamPrivate>>" erfordert eine DLL-Schnittstelle, die von Clients von class "log4cxx::helpers::FileInputStream" verwendet wird C:\Users\tschoening\Documents\Svn\Src\Libs\trunk\C++\Logging\log4cxx\master\src\out\build\x64-Debug\src C:\Users\tschoening\Documents\Svn\Src\Libs\trunk\C++\Logging\log4cxx\master\src\src\main\include\log4cxx\helpers\fileinputstream.h 40
And some error, while I fixed some others in the attachment already:
> Schweregrad Code Beschreibung Projekt Datei Zeile Unterdrückungszustand
> Fehler LNK2019 Verweis auf nicht aufgelöstes externes Symbol ""public: static class log4cxx::helpers::Class const & __cdecl log4cxx::rolling::RollingFileAppenderSkeleton::getStaticClass(void)" (?getStaticClass@RollingFileAppenderSkeleton@rolling@log4cxx@@SAAEBVClass@helpers@3@XZ)" in Funktion ""public: virtual void const * __cdecl log4cxx::rolling::RollingFileAppenderSkeleton::cast(class log4cxx::helpers::Class const &)const " (?cast@RollingFileAppenderSkeleton@rolling@log4cxx@@UEBAPEBXAEBVClass@helpers@3@@Z)". C:\Users\tschoening\Documents\Svn\Src\Libs\trunk\C++\Logging\log4cxx\master\src\out\build\x64-Debug\src C:\Users\tschoening\Documents\Svn\Src\Libs\trunk\C++\Logging\log4cxx\master\src\out\build\x64-Debug\src\timebasedrollingpolicy.cpp.obj 1
Is the following line correct in your docs?
> std::unique_ptr<AppenderSkeletonPrivate> m_priv;
https://github.com/apache/logging-log4cxx/blame/LOGCXX-510/src/site/markdown/library-design.md#L88
Should this be the following instead? Looks like a copy&paste error.
> std::unique_ptr<ParentPrivate> m_priv;
Some naming discussion: How about using "ParentPriv" and
"ParentPriv.h" instead of heaving "ParentPrivate", "Parent-private.h"
and "m_priv". It's shorter, "-private" doesn't follow QT conventions
anyway and the abbreviation "priv" is used already as well.
When looking at your code, you are using some "XyPriv" and "xy_priv.h"
and some "XyPrivate" and "xy_priv.h" already. For existing classes, I
suggest keeping "xy_priv.h", but for newer classes "XyPriv" and
"xyPriv.h" read better in my opinion. So I would suggest that in your
new docs.
About inheritance: Wouldn't it be better in most cases to provide
protected or public getters in base classes anyway instead of
accessing private data structs?
I understand your example to show how it could be done in case no such
getters are available or should be used for some reason. Or is that
example showing how it needs to be done always regardless how the
private data is accessed by subclasses?
> #define priv static_cast<AndFilterPrivate*>(m_priv.get())
Does it make sense to mention something like that in your new docs?
Is it possible to make it a (generic?) getter or alike instead of a
macro?
Mit freundlichen Grüßen
Thorsten Schöning
--
AM-SoFT IT-Service - Bitstore Hameln GmbH
Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK
E-Mail: Thorsten.Schoening@AM-SoFT.de
Web: http://www.AM-SoFT.de/
Tel: 05151- 9468- 0
Tel: 05151- 9468-55
Fax: 05151- 9468-88
Mobil: 0178-8 9468-04
AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln
AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska