Skip to content

MinGW - Bugfix - C99 compliance (overide and replace the pull request #/1037)#1043

Closed
philippefoubert wants to merge 1 commit intoopencv:masterfrom
philippefoubert:branch_mingw_c99
Closed

MinGW - Bugfix - C99 compliance (overide and replace the pull request #/1037)#1043
philippefoubert wants to merge 1 commit intoopencv:masterfrom
philippefoubert:branch_mingw_c99

Conversation

@philippefoubert
Copy link
Copy Markdown
Contributor

Some functions provided by Microsoft does not work as described in C99 specifications. For example function 'vsnprintf' does not return number of characters that would have been printed but only '-1' if the format does not fit in the given field.
Mingw provides possibility to assume ANSI I/O standards are preferred over Microsoft's. This is enabled by

define USE_MINGW_ANSI_STDIO 1

before including any system libraries. This way, without modifying the code, the call to "sprintf" (Microsoft's implementation) is replaced by a call to "mingw_sprintf" which is C99 compliant.

Some functions provided by Microsoft does not work as described in C99 specifications. For example function 'vsnprintf' does not return number of characters that would have been printed but only '-1' if the format does not fit in the given field.
Mingw provides possibility to assume ANSI I/O standards are preferred over Microsoft's. This is enabled by
#define USE_MINGW_ANSI_STDIO 1
before including any system libraries. This way, without modifying the code, the call to "sprintf" (Microsoft's implementation) is replaced by a call to "mingw_sprintf" which is C99 compliant.
@SpecLad
Copy link
Copy Markdown

SpecLad commented Jun 25, 2013

I don't understand, are you talking about sprintf or vsnprintf here? In either case, we support Microsoft's compiler, so we have be able to work with whatever implementations they give us anyway; thus I don't see any reason to enable this.

@philippefoubert
Copy link
Copy Markdown
Contributor Author

You shoud try to save a cv:Mat in a XML file using FileStorage with an OpenCV library built using MinGW. You will see that the content of the file is not what you would have expected.

There are many posts on the web around this problem. You could for example take a look at:

It took me some time to find where the problem came from and a smart way to solve it. That's way I opened this PR.

@ghost ghost assigned SpecLad Jun 25, 2013
@SpecLad
Copy link
Copy Markdown

SpecLad commented Jun 26, 2013

Are you suggesting that OpenCV sometimes invokes sprintf in a way that is not allowed by C90? If so, that is a bug in OpenCV and those invocations should be fixed. We do not need C99 semantics, because we're not supposed to rely on them.

@mdim
Copy link
Copy Markdown
Contributor

mdim commented Jul 2, 2013

@philippefoubert is Roman's assumption about sprintf true?

@philippefoubert
Copy link
Copy Markdown
Contributor Author

The file "persistence.cpp" (location of FileStorage) could be the object of a code review to check if it really doesn't rely on C99 compliance.

All I can say is that the added define is solving my problem: it means that OpenCV is calling Microsoft functions (throught the operating system libraries) that do not follow the C99 specifications.

I don't know if we should change anything in the code since it is a Microsoft non conformity toward the standard and the proposed modification is solving the problem but someone should check that the problem only appears when compiling with MinGW.

@SpecLad
Copy link
Copy Markdown

SpecLad commented Jul 3, 2013

I briefly checked all uses of *printf in persistence.cpp, but I didn't find anything out of the ordinary.

Could you elaborate on what difference there is between the expected and actual outputs? I'm not on Windows, so I can't easily try this myself.

@apavlenko
Copy link
Copy Markdown
Contributor

@philippefoubert could you provide the difference between cv::Mat saved with and without your change with MinGW?
we can't merge your change until we understand what it solves!

@apavlenko
Copy link
Copy Markdown
Contributor

@philippefoubert I will have to close the pull request if there is no problem statement that is solved by it.

@philippefoubert
Copy link
Copy Markdown
Contributor Author

Sorry for my late answer, i was off. This pull request is relative to a correction I made about two years ago. Since then, I changed my development environnement. I am now working on a Windows Seven 64 bits operating system (instead of Windows XP 32 bits). I am now using gcc (x86_64-w64-mingw32) version 4.8.0 (instead of an older 32 bits version).

As you suggested, I wrote some basic code to show the difference between the cv::Mat saved with and without my change with MinGW: the problem seems not to be present any more.

So I looked further in MinGW64 to understand why it has changed. The reason seems to be that the "os_defines.h" header file has changed. Now, we can find in this file (lines 47 to 51):
"// Make sure that POSIX printf/scanf functions are activated. As
// libstdc++ depends on POSIX-definitions of those functions, we define
// it unconditionally.
#undef __USE_MINGW_ANSI_STDIO
#define __USE_MINGW_ANSI_STDIO 1"

I looked on the web: this correction in MinGW64 has been done in december 2011 (see http://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=4f5aba356dc05f1bbb3aa498fa6c482631531bbb).

So, my modification seems only to be usefull with older versions of the compiler (i am sorry but i have not kept any xml file illustrating the problem...) but it seems useless with the latest ones.

It is up to you to choose adding it or not. From my point of view, since some of my collegues are still using older versions of MinGW, i will keep it in my own custom version of the library.

@apavlenko
Copy link
Copy Markdown
Contributor

Ok, thanks for the explanation.
I think there is no need to add to OpenCV a workaround for a 3rd-party bug fixed about 2 years ago...

@apavlenko apavlenko closed this Aug 4, 2013
@philippefoubert philippefoubert deleted the branch_mingw_c99 branch July 30, 2017 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants