Re: [gpsd-dev] compile issue: header not necessarily included for atomic_thread_fence

[Gary E. Miller]

Yo Gergely!

On Thu, 26 Nov 2015 12:17:31 +0800
Gergely Imreh <address@hidden> wrote:

> > First thing is we need to know you OS, distro, CC version, etc.  
> 
> ArchLinux, GCC 5.2.0, scons v2.4.0.rel_2.4.0:3365:9259ea1c13d7.
> Any other info is needed?

Intel CPU?  What version of glibc?

> > Possibly, but I'd like to know what the __cplusplus is from.  I
> > have no idea what OS, dirstro, CC would set that and that is way
> > too generic a test for my taste.  
> 
> Looking it up, __cplusplus is a standard macro in GCC and "defined
> when the C++ compiler is in use."
> https://gcc.gnu.org/onlinedocs/cpp/Standard-Predefined-Macros.html

Right, which is orthongonal to if you system supports stdatomic.h.

Doing c++ tells you nothing about how your compiler supports atomics.

> The reason I have that check in the patch, because there's already the
> exact same check earlier in compilers.h (lines 68-72):

Yea, you already explained that.  But one piece of bad code is not
an excuse for a second one.

If is right to test for C++, but only to include the file as a C
header and not a C++ header.

Another test is needed for if you compiler supports stdatomic.h.  That
is the HAVE_STDATOMIC_H.


> Because of this, it's possible that the checks evaluate differently,
> and atomic_thread_fence is called without the header being included.

Yes, but your patch is to NOT include the header, and we want it included.

> Hence the patch.

Hence the need or a good patch.  One that included stdatomic.h in the
right way, when needed.

Both where stdatomic.h is included, and where it is used need to
be on the same page, so they need t both be fixed.

> That __cplusplus check for the #include was added in commit
> 79f6d9133378325d70a92e66f7352c1becefbb88 and there's no rationale
> given in the commit message why it is needed (just a sign-off by you).

Yeah, shit happens.  Often my own.

I'm thinking the include part needs to look mmore like this:


    #ifdef HAVE_STDATOMIC_H
    #if !defined(__COVERITY__)
    extern "C" {
    # endif

    #include <stdatomic.h>
    # ifdef __cplusplus
    }
    # endif

    #endif /* __COVERITY__ 
    #endif /* HAVE_STDATOMIC_H */

That way if you have stdatomic.h it gets included, in a way that is
legal for C++.  Then the other place needs to match.

RGDS
GARY
---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97703
        address@hidden  Tel:+1 541 382 8588

pgpAkSjZVhpQV.pgp
Description: OpenPGP digital signature