Inline functions, ODR, logging

[Andreas]

Hi all,

I wonder how to use logging in inline functions without violating ODR.
To make clear what I mean, please consider the following example:

<f.hh>
#ifndef F_INCLUDED_
#define F_INCLUDED_ 1

#if ! defined(FOOBAR)
#define FOOBAR "foobar"
#endif

#include <iostream>


struct Bogus
{
void p(void) const;
};

static inline void Bogus::p(void) const
{
std::cout << FOOBAR << std::endl;
}

#endif // F_INCLUDED_
</f.hh>

<f1.hh>
#ifndef F1_INCLUDED_
#define F1_INCLUDED_ 1

void f1(void);

#endif // F1_INCLUDED_
</f1.hh>

<f1.cc>
#define FOOBAR "foo"

#include "f1.hh"
#include "f.hh"

void f1(void)
{
Bogus b;

b.p();
}
</f1.cc>

<f2.cc>
#define FOOBAR "bar"

#include "f1.hh"
#include "f.hh"

void f2(void)
{
Bogus b;

b.p();
}

int main(int argc, char* argv[])
{
f1();
f2();

return 0;
}
</f2.cc>

AFAICT, this violates ODR because of the differing expansions of FOOBAR
in the two translation units ("g++ -O0" prints "foo" and "foo",
depending on the sequence of compiling the translation units, "g++ -O3"
prints "foo" and "bar").

I assume the usage of __FILE__ (or __DATE__) in Bogus::p would have
similar problems.

So my question is: if you are not allowed to use __FILE__ or similar
macros (like in the example above), how do you handle logging from C++
inline functions?

Is it for instance ok to use a file-scope static "char const*" variable
as the module name instead of a macro?

I also tried to give Bogus::p internal linkage by declaring _its
definition_ "static inline", which didn't work ...

The corresponding example in C using a "static inline" function works as
expected.


TIA,

Andreas

--
What do you mean? An African or a European swallow?

[Paavo Helde]

On 16.11.2016 1:54, Andreas wrote:
> Hi all,
>
> I wonder how to use logging in inline functions without violating ODR.
> To make clear what I mean, please consider the following example:
>
> <f.hh>
> #ifndef F_INCLUDED_
> #define F_INCLUDED_ 1
>
> #if ! defined(FOOBAR)
> #define FOOBAR "foobar"
> #endif
>
> #include <iostream>
>
>
> struct Bogus
> {
> void p(void) const;
> };
>
> static inline void Bogus::p(void) const
> {
> std::cout << FOOBAR << std::endl;
> }

The (void) above is a C-ism.

'static' as used above does not compile and is not needed.

To get what you apparently want, you could use:

struct Bogus {
void p(const char* arg = FOOBAR) const;
};

inline void Bogus::p(const char* arg) const {
std::cout << arg << std::endl;
}

In general, I'm sure you know macros are evil and should be used as
little as possible. Some predefined macros like __LINE__ are useful
indeed, but note that using __LINE__ *inside* a logger function would
have no point. You need to pass it in via an argument, but using the
default value trick as above will not help with __LINE__. Instead,
logging and asserts are often performed via function-style macros, one
reason being to able to expand __LINE__ in the correct place, e.g.:

#define LOGGER(msg) { Logger::Log(__FILE__, __LINE__, msg); }

hth
Paavo

[Öö Tiib]

On Wednesday, 16 November 2016 01:55:45 UTC+2, Andreas wrote:
> Hi all,
>
> I wonder how to use logging in inline functions without violating ODR.
> To make clear what I mean, please consider the following example:

Your example still does not explain *why* you wrote that strange code
in the way you wrote and what you expected it to do.

> AFAICT, this violates ODR because of the differing expansions of FOOBAR
> in the two translation units ("g++ -O0" prints "foo" and "foo",
> depending on the sequence of compiling the translation units, "g++ -O3"
> prints "foo" and "bar").

Yes, you violate standard with that code.

>
> I assume the usage of __FILE__ (or __DATE__) in Bogus::p would have
> similar problems.

Yes, whatever ways of achieving that same inline function compiles
to something different in different translation units is defect.

>
> So my question is: if you are not allowed to use __FILE__ or similar
> macros (like in the example above), how do you handle logging from C++
> inline functions?

Typically the things like __FILE__ or __LINE__ are instrumented into
logged line using function-like macro at line where logging occurs.

>
> Is it for instance ok to use a file-scope static "char const*" variable
> as the module name instead of a macro?

No.

>
> I also tried to give Bogus::p internal linkage by declaring _its
> definition_ "static inline", which didn't work ...
>
> The corresponding example in C using a "static inline" function works as
> expected.

Static in class scope does not mean static linkage
but class static.

[Scott Lurndal]

Paavo Helde <myfir...@osa.pri.ee> writes:

>> static inline void function(void) const

>
>The (void) above is a C-ism.

so what? It's both legal and expressive.

variable++ is also a C-ism.

[David Brown]

On 16/11/16 08:27, Paavo Helde wrote:
> On 16.11.2016 1:54, Andreas wrote:
>> Hi all,
>>
>> I wonder how to use logging in inline functions without violating ODR.
>> To make clear what I mean, please consider the following example:
>>
>> <f.hh>
>> #ifndef F_INCLUDED_
>> #define F_INCLUDED_ 1
>>
>> #if ! defined(FOOBAR)
>> #define FOOBAR "foobar"
>> #endif
>>
>> #include <iostream>
>>
>>
>> struct Bogus
>> {
>> void p(void) const;
>> };
>>
>> static inline void Bogus::p(void) const
>> {
>> std::cout << FOOBAR << std::endl;
>> }
>
> The (void) above is a C-ism.

Yes, and it is IMHO a good one. Making "void foo()" in C mean a
function with a variable or unspecified number of arguments was a
terrible idea. Changing it in C++ to mean no arguments was only a
little less bad - it should simply have been a syntax error. Then
people would have been forced to use the clear syntax of "void
foo(void)" that is consistent with C, and it would have avoided the
"most vexing parse" in C++.

>
> 'static' as used above does not compile and is not needed.
>
> To get what you apparently want, you could use:
>
> struct Bogus {
> void p(const char* arg = FOOBAR) const;
> };
>
> inline void Bogus::p(const char* arg) const {
> std::cout << arg << std::endl;
> }
>
> In general, I'm sure you know macros are evil and should be used as
> little as possible. Some predefined macros like __LINE__ are useful
> indeed, but note that using __LINE__ *inside* a logger function would
> have no point. You need to pass it in via an argument, but using the
> default value trick as above will not help with __LINE__. Instead,
> logging and asserts are often performed via function-style macros, one
> reason being to able to expand __LINE__ in the correct place, e.g.:
>
> #define LOGGER(msg) { Logger::Log(__FILE__, __LINE__, msg); }
>

Macros are not evil. Using macros when there are better solutions is
evil, and /abusing/ macros is evil.

Changing the meaning of an include file depending on the definition of a
macro is evil, and that is the root of the problem here.

[Alf P. Steinbach]

On 16.11.2016 14:50, Scott Lurndal wrote:
> Paavo Helde <myfir...@osa.pri.ee> writes:
>
>>> static inline void function(void) const
>>
>> The (void) above is a C-ism.
>
> so what? It's both legal and expressive.

It doesn't express anything in C++.

> variable++ is also a C-ism.

No, that's the same in both languages.


Cheers & hth.,

- Alf

[Andreas]

Paavo Helde <myfir...@osa.pri.ee> writes:

> struct Bogus {
> void p(const char* arg = FOOBAR) const;
> };
>
> inline void Bogus::p(const char* arg) const {
> std::cout << arg << std::endl;
> }

Ah, that's interesting. But I do not want to require an additional
parameter for each inline function, because that parameter would have no
relationship with the responsibilities of the function.

> #define LOGGER(msg) { Logger::Log(__FILE__, __LINE__, msg); }
>
> hth
> Paavo

Yes, it does indeed, thanks. You made it clear to me that my posting
was misleading because I did not explain my point well enough.

Bogus::p was _not_ meant to replace a logger as your macro above, but as
an inline function using such macro. Like so:

inline void Bogus::p() const {
LOGGER("Bogus::p was called!");
}

As your LOGGER macro uses __FILE__, it will violate ODR when used in
Bogus::p in the same way that the use of FOOBAR will violate it. I'm
looking for a viable alternative for such a macro for use in inline
functions.

[Andreas]

"Alf P. Steinbach" <alf.p.stein...@gmail.com> writes:

> On 16.11.2016 14:50, Scott Lurndal wrote:
>> Paavo Helde <myfir...@osa.pri.ee> writes:
>>
>>>> static inline void function(void) const
>>>
>>> The (void) above is a C-ism.
>>
>> so what? It's both legal and expressive.
>
> It doesn't express anything in C++.

Hmm, it at least makes explicit that the function does have no
arguments. Such redundancies are often useful, IMHO. For example the
braces in

if (test())
{
do_something();
}

do not express anything but I really prefer them being used. After all
code is not just read by compilers but (more importantly) by human
beings who sometimes may appreciate redundancies/explicitness.

[Paavo Helde]

On 17.11.2016 0:15, Andreas wrote:
> Paavo Helde <myfir...@osa.pri.ee> writes:
>

>> #define LOGGER(msg) { Logger::Log(__FILE__, __LINE__, msg); }
>>
>

> Yes, it does indeed, thanks. You made it clear to me that my posting
> was misleading because I did not explain my point well enough.
>
> Bogus::p was _not_ meant to replace a logger as your macro above, but as
> an inline function using such macro. Like so:
>
> inline void Bogus::p() const {
> LOGGER("Bogus::p was called!");
> }
>
> As your LOGGER macro uses __FILE__, it will violate ODR when used in
> Bogus::p in the same way that the use of FOOBAR will violate it.

No it doesn't violate ODR, __FILE__ will be the same in all compilation
units (namely, "f.hh").

[Andreas]

David Brown <david...@hesbynett.no> writes:

> Changing the meaning of an include file depending on the definition of a
> macro is evil, and that is the root of the problem here.

While I agree to your considerations about macros, this statement seems
to be a little too broad for me to buy it.

I think it is quite common to change the meaning of include files
depending on a macro definition. Some examples are:

* macros which expand to declspec(dllimport) or declspec(dllexport) (or
corresponding __attribute__ annotations) depending on from where they
are included
* adding or removing declarations (like typedefs) to/from include files
depending on macro deinitions

For instance glibc headers are full of those.

[Fred.Zwarts]

"Andreas" schreef in bericht news:87k2c31...@starless.invalid...

>
>"Alf P. Steinbach" <alf.p.stein...@gmail.com> writes:
>
>> On 16.11.2016 14:50, Scott Lurndal wrote:
>>> Paavo Helde <myfir...@osa.pri.ee> writes:
>>>
>>>>> static inline void function(void) const
>>>>
>>>> The (void) above is a C-ism.
>>>
>>> so what? It's both legal and expressive.
>>
>> It doesn't express anything in C++.
>
>Hmm, it at least makes explicit that the function does have no arguments.

In C++ also () makes explicit that the function has no arguments.
It is a matter of taste whether it is more clear. For me it is clear that ()
means no arguments, whereas (void) looks as if there is one argument of type
void. Then I realize that there is no such type void and I wonder whether
(void *) was meant. Then I realize that in a function declaration void means
no argument. For me it takes more time to get the right meaning of (void)
than of ().

[Ian Collins]

glibc headers are part of the implementation, so they can do what they
like...

--
Ian

[David Brown]

On 16/11/16 23:52, Andreas wrote:
> David Brown <david...@hesbynett.no> writes:
>
>> Changing the meaning of an include file depending on the definition of a
>> macro is evil, and that is the root of the problem here.
>
> While I agree to your considerations about macros, this statement seems
> to be a little too broad for me to buy it.
>
> I think it is quite common to change the meaning of include files
> depending on a macro definition. Some examples are:
>
> * macros which expand to declspec(dllimport) or declspec(dllexport) (or
> corresponding __attribute__ annotations) depending on from where they
> are included

I would say that sounds evil too. But I have not worked with C++ (or C)
on Windows, at least not for writing dlls, so I can't say if there is a
better way to do it. Maybe I would call it a "necessary evil".

> * adding or removing declarations (like typedefs) to/from include files
> depending on macro deinitions
>
> For instance glibc headers are full of those.
>

Let me be a little narrower and say that I think changing the meaning
/within a program/ of an include file depending on the definition of a
macro is evil. Changing the details depending on things like
pre-defined macros from the compiler is fair enough - the glibc headers
do that in order to be portable across a range of targets. And changing
depending on compiler flags (such as "-D" flags to define macros used
for options) is also okay, if it is well documented. It is also fine to
have something like an "options.h" file that is included by other
headers and used to change the effects of those headers.

What I don't like is when the meaning of the same header-defined
identifier (type, function, macro, whatever) is different in different
parts of the code, because of the order of #defines and #includes. It
makes things inconsistent, and hard to follow, and leads to incorrect
assumptions.

(And to be clear, just because something is "evil" does not mean it
should not be used. Sometimes evil practices are unavoidable, or the
alternatives are even worse.)

[Paavo Helde]

On 17.11.2016 0:15, Andreas wrote:

> Paavo Helde <myfir...@osa.pri.ee> writes:
>
>> struct Bogus {
>> void p(const char* arg = FOOBAR) const;
>> };
>>
>> inline void Bogus::p(const char* arg) const {
>> std::cout << arg << std::endl;
>> }
>
> Ah, that's interesting. But I do not want to require an additional
> parameter for each inline function, because that parameter would have no
> relationship with the responsibilities of the function.

You want to use some translation-unit information in the function which
is not translation-unit specific. So this means the data needs to
somehow arrive there. There is no magic way, the function is compiled
into a binary code which is not related to any translation unit, so
there is no way how it could automatically access this information.

You need to pass in such info via parameters (including *this), via a
global variable (if your app is single-threaded) or via thread-specific
variables (if your app is multi-threaded).

Another way is to make your function translation-unit specific (by
declaring the whole class Bogus in an anonymous namespace, for example).
This means that all code is duplicated in each translation unit, not
sure if this is what you want.

You can't eat your cake and have it too!

hth
Paavo

[Alf P. Steinbach]

On 17.11.2016 09:50, David Brown wrote:
> On 16/11/16 23:52, Andreas wrote:
>> David Brown <david...@hesbynett.no> writes:
>>
>>> Changing the meaning of an include file depending on the definition of a
>>> macro is evil, and that is the root of the problem here.
>>
>> While I agree to your considerations about macros, this statement seems
>> to be a little too broad for me to buy it.
>>
>> I think it is quite common to change the meaning of include files
>> depending on a macro definition. Some examples are:
>>
>> * macros which expand to declspec(dllimport) or declspec(dllexport) (or
>> corresponding __attribute__ annotations) depending on from where they
>> are included
>
> I would say that sounds evil too. But I have not worked with C++ (or C)
> on Windows, at least not for writing dlls, so I can't say if there is a
> better way to do it. Maybe I would call it a "necessary evil".
>
>> * adding or removing declarations (like typedefs) to/from include files
>> depending on macro deinitions
>>
>> For instance glibc headers are full of those.
>>
>
> Let me be a little narrower and say that I think changing the meaning
> /within a program/ of an include file depending on the definition of a
> macro is evil.

Well that depends. Consider `<assert.h>`. It's designed to change its
meaning in the middle of a translation unit, depending on `NDEBUG`. The
standard has extra language to make sure that implementors get it right,
that that's really the intention.

But it does clash directly with the notion of precompiled headers: AFAIK
there's no way to precompile that changing behavior.

And given the sorry state of C++ compiler technology, the 1950's batch
job idea that all common compilers are based on, with companies
resorting to build farms and whatnot to somewhat alleviate the problems,
anything that clashes with faster-build techniques is ungood.

As I see it.

:)


Cheers!,

- Alf

[David Brown]

Yes, I know about <assert.h>, and I don't like the way it depends on
NDEBUG. I would much prefer that disabling assertions was done by:

#include <assert.h>
#define NDEBUG 1

rather than

#define NDEBUG 1
#include <assert.h>


When the C library was first conceived, compilers were not good enough
at things like dead code elimination for this to be an efficient
solution. But that is not the case now.

>
> But it does clash directly with the notion of precompiled headers: AFAIK
> there's no way to precompile that changing behavior.

Indeed. Precompiled headers usually rely on having exactly the same
setup (based on a hash of the pre-processed source, I think), or they
are considered invalid.

>
> And given the sorry state of C++ compiler technology, the 1950's batch
> job idea that all common compilers are based on, with companies
> resorting to build farms and whatnot to somewhat alleviate the problems,
> anything that clashes with faster-build techniques is ungood.
>
> As I see it.
>

I agree, though I am more concerned about the possibility of making
mistakes or accidentally breaking the ODR than the speed of compilation.

[Andreas]

Paavo Helde <myfir...@osa.pri.ee> writes:

> No it doesn't violate ODR, __FILE__ will be the same in all
> compilation units (namely, "f.hh").

Ooops, you are right; I've obviously forgotten that __FILE__ is magic,
i.e. nothing I can do as application programmer. Sorry for having been
dense ...