libuemf include file issue

Bug #1241797 reported by David Mathog
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Inkscape
Fix Released
Undecided
David Mathog

Bug Description

Originally the file src/libuemf/uemf.h included some defines that used C99 constructs like this:

   #define U_RGB(r,g,b) (U_COLORREF){r,g,b, 0}

These were triggering compiler warnings so Krzysztof at some point changed them in trunk to be like this:

   #define U_RGB(r,g,b) (U_COLORREF){(uint8_t)(r), (uint8_t)(g), (uint8_t)(b), 0}

which eliminated the warnings.

However, after further research it seems that C99 compound literals really should never be seen by a C++ compiler. In C++03 it should not even compile, although g++ does for some reason, probably to paper over this exact issue, and in C++11 it means something that is not quite the same thing. The current set of compilers appear to do something useful with the code, but that condition is not guaranteed to continue for future compiler releases, so if it is not fixed it might cause mysterious problems in future.

The attached patch moves the C99 feature inside libUEMF C functions, and changes #defines like U_RGB() to reference those functions. That should completely eliminate the issue, since the C++ compiler will never see a compound literal.

The patch also removes a couple of defines that were never used and have been hanging around since a very early version of libUEMF.

Related observation: when built with "make V=1" I see that the -std switch is never applied. This should be -std=c99 for the libUEMF pieces, and it should be something, probably -std=c++98 for g++. Not specifying the language standard in the build is just begging for problems, because developers will start using features in newer compiler releases which were not available on older ones. The default standard for g++ is gnu++98, which may or may not be what is really intended.

Tags: build emf
Revision history for this message
David Mathog (mathog) wrote :
su_v (suv-lp)
tags: added: build emf
Kris (kris-degussem)
Changed in inkscape:
milestone: none → 0.49
Revision history for this message
David Mathog (mathog) wrote :

Somebody please test on OS X, and if OK there, commit.

There is no specific test for this patch, it should not change Inkscape behavior.
The attached test EMF file should open the same before and after the test.
To verify that, try this test (trunk r12809 or greater):

src/inkscape --file /tmp/test_libuemf_ref.emf --export-emf=/tmp/test.emf
mv /tmp/test.emf /tmp/test1.emf
patch -p0 < changes_2013_10_18b.patch
make
src/inkscape --file /tmp/test_libuemf_ref.emf --export-emf=/tmp/test.emf
mv /tmp/test.emf /tmp/test2.emf
diff /tmp/test1.emf /tmp/test2.emf

The two files should be identical.

[Note: the file name is stored in the output file, which is why the copies must be renamed outside of inkscape.]

They _might_ differ if the patch for bug #1250250 has not yet been applied, but I think not. That bug causes the drawing to be offset vertically when it opens. However, in several cycles of

touch src/main.cpp
make
src/inkscape --file /tmp/test_libuemf_ref.emf --export-emf=/tmp/test.emf
mv /tmp/test.emf /tmp/test2.emf
diff /tmp/test1.emf /tmp/test2.emf

diff never saw any changes.

Revision history for this message
Martin Owens (doctormo) wrote :

Testing works here, I'm happy to apply the patch, see r12926.

Please assign mathog to this bug.

Changed in inkscape:
status: New → Fix Committed
jazzynico (jazzynico)
Changed in inkscape:
assignee: nobody → David Mathog (mathog)
Bryce Harrington (bryce)
Changed in inkscape:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.