Bug 103621

Please don't attach patches as a zip file, makes it unnecessarily hard to use :)

Why do you need the __cplusplus guards, poppler is all C++, no ?

Also we will need your full name for copyright attribution.

Functions in the goo\glibc.h are defined as C-function, so somewhere should be a C-file with these functions, but the Poppler's source contains a C++ compiled files where you declare the C-functions. That is why I used  __cplusplus guards in header - for further rename of the C++ compiled files with C-function declarations to .c files

Other reason to use __cplusplus guards is codestyle - same sense as for static_cast (sometimes) - ugly code for ugly design. This codestyle should help to understand how to make the design better or warn user/coder about something unusual.

Best regards,
 Aleksey Nikolaev

Created attachment 135345 [details] [review]
strtok_r fix on MSVC

Created attachment 135346 [details] [review]
strtok_r fix on MSVC

(In reply to mr NAE from comment #2)
> Functions in the goo\glibc.h are defined as C-function, so somewhere should
> be a C-file with these functions, but the Poppler's source contains a C++
> compiled files where you declare the C-functions. That is why I used 
> __cplusplus guards in header - for further rename of the C++ compiled files
> with C-function declarations to .c files
> 
> Other reason to use __cplusplus guards is codestyle - same sense as for
> static_cast (sometimes) - ugly code for ugly design. This codestyle should
> help to understand how to make the design better or warn user/coder about
> something unusual.
> 
> Best regards,
>  Aleksey Nikolaev

You really lost me there, are you saying "this is unneeded but i just added them because i think it makes sense style wise"?

I'd prefer to see less include guards all over the code. They make the code hard to read and more brittle as not all combinations of the code paths get tested regularly.

Currently the only C code in poppler is the glib demo. It does not currently use goo/glibc.h. If this changes we can add the include guards but I don't see the point of adding them now.

I like Jean Ghali's suggestion of renaming glibc*.cc to .c I did a test build with this change (and removed the extern "C") and it built fine.

The glibc.cc stuff is meant to be emulations of C functions so it makes sense to build it as C code.

Created attachment 135435 [details] [review]
strtok_r fix on MSVC

but we don't use strtok_r in GooString.h

Please ignore my patch if you wish

When the  goo\glibc.h is used the compiler cannot find the definition and declaration of strtok_r, so please fix it.

(In reply to Adrian Johnson from comment #7)
> I like Jean Ghali's suggestion of renaming glibc*.cc to .c I did a test
> build with this change (and removed the extern "C") and it built fine.
> 
> The glibc.cc stuff is meant to be emulations of C functions so it makes
> sense to build it as C code.

If glic.cc is renamed to .c, removing the extern "C" in glibc.h will not work. In this case, the __cplusplus guard becomes required in glibc.h as glibc.h gets then included in C and C++ code.

Created attachment 135534 [details] [review]
Include glibc.h where needed

Can we at least start by including glibc.h where strtok_r is used? Here is a patch for that. We can discuss how to fix the linker issue afterwards.

pushed.

I still need to understand why there's a need to wrap code in C-aware-stuff if everything in poppler is C++

If everything in poppler is C++, then the extern "C" stuff in goo/glibc.h is unneeded in the first place.

If glibc.h is kept as is, and assuming glibc*.cc files are compiled as C++, then the extern "C" stuff is required in glibc.cc and glibc_strtok_r.cc so that the function name mangling be consistent between the functions declaration and the implementation. Otherwise a function funcName(...) declared inside an extern "C" and a function funcName(...) implemented outside an extern "C" are just two different functions for the C++ compiler and linker. Adding __cplusplus guards in glibc*.cc files ensures that the function name mangling is consistent between the declaration and implementation whether those files are compiled as C or C++.

In the end, the first decision to take is whether glibc*.cc files continue to be compiled as C++ or if they should be compiled as C, in which case it is better to rename those files as *.c.

My vote is removing extern "C" stuff in goo/glibc.h if that fixes your problem.

Oops, sorry, I lost track of this. Removing the extern "C" stuff in goo/glibc.h is fine with me.

ok, pushed the removal of extern C

You missed #include "config.h" (or #include "glibc.h") in  goo/glibc_strtok_r.cc before #ifndef HAVE_STRTOK_R that is why HAVE_STRTOK_R is always undefined

That's been fixed as part of #104173