Bug 103621

Summary: MSVC does not find strtok_r
Product: poppler Reporter: mr NAE <nae202>
Component: generalAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: x86-64 (AMD64)   
OS: Windows (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: strtok_r fix on MSVC
strtok_r fix on MSVC
strtok_r fix on MSVC
strtok_r fix on MSVC
Include glibc.h where needed

Description mr NAE 2017-11-08 15:29:33 UTC
Created attachment 135304 [details]
strtok_r fix on MSVC

I found a problem that MSVC does not find strtok_r when compile Poppler.
Comment 1 Albert Astals Cid 2017-11-09 08:41:26 UTC
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.
Comment 2 mr NAE 2017-11-09 12:19:49 UTC
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
Comment 3 mr NAE 2017-11-09 12:22:08 UTC
Created attachment 135345 [details] [review]
strtok_r fix on MSVC
Comment 4 mr NAE 2017-11-09 12:34:10 UTC
Created attachment 135346 [details] [review]
strtok_r fix on MSVC
Comment 5 Albert Astals Cid 2017-11-11 23:13:29 UTC
(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"?
Comment 6 Adrian Johnson 2017-11-11 23:22:08 UTC
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.
Comment 7 Adrian Johnson 2017-11-12 01:19:28 UTC
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.
Comment 8 mr NAE 2017-11-13 11:50:14 UTC
Created attachment 135435 [details] [review]
strtok_r fix on MSVC
Comment 9 Albert Astals Cid 2017-11-13 21:17:28 UTC
but we don't use strtok_r in GooString.h
Comment 10 mr NAE 2017-11-14 10:45:00 UTC
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.
Comment 11 Jean Ghali 2017-11-17 00:31:30 UTC
(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.
Comment 12 Jean Ghali 2017-11-17 00:36:20 UTC
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.
Comment 13 Albert Astals Cid 2017-11-17 22:15:17 UTC
pushed.

I still need to understand why there's a need to wrap code in C-aware-stuff if everything in poppler is C++
Comment 14 Jean Ghali 2017-11-18 00:05:04 UTC
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.
Comment 15 Albert Astals Cid 2017-11-20 21:19:51 UTC
My vote is removing extern "C" stuff in goo/glibc.h if that fixes your problem.
Comment 16 Jean Ghali 2017-12-07 22:30:20 UTC
Oops, sorry, I lost track of this. Removing the extern "C" stuff in goo/glibc.h is fine with me.
Comment 17 Albert Astals Cid 2017-12-12 23:08:29 UTC
ok, pushed the removal of extern C
Comment 18 mr NAE 2017-12-18 11:38:01 UTC
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
Comment 19 Albert Astals Cid 2017-12-18 23:42:51 UTC
That's been fixed as part of #104173

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.