gdal 1.5.0 fails building with gcc 4.3

[frankie]

See

 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=462709

for more information. A patch against 1.5.0 would be nice because Debian is going to move to 4.3 as default compiler in Lenny.

[warmerdam]

It seems the transform() function isn't working properly - likely due to issue with namespaces or something in how it is accessed in the ILWIS code. I'm going to take a crack at removing use of transform which appears to only be used to convert strings to lower case.

[warmerdam]

I have reviewed the code, and the tolower transformation of the strings doesn't matter since all subsequent testing of them is done using EQUAL() which is case insensitive anyways.

I have removed the transform() calls in trunk (r13614) and 1.5 branch (r13615).

In the process of doing this I wrote a small test script for ilwis format and seem to have found some problems with the driver, but I'll address these in separate bug reports.

[warmerdam]

Bas Retsios wrote:

Hi! Yes, transform is used for converting the string to lower case. It is the "normal" way to do this with STL strings. This transform() can be rewritten as a simple for-loop (with STL iterator), comparing each char to 'A' and 'Z' and subtracting 'A'-'a' if true. E.g.: void transform(string & str) { for (string::iterator it = str.begin(); it != str.end(); ++it) if ((*it) >= 'A' && (*it) <= 'Z') (*it) += 'a' - 'A'; } I didn't syntax-check it, but this should be quite close. Then the #include <algorithm> (that contains the STL transform) can be removed from ilwisdataset.h If STL poses a problem we could attempt rewriting the ILWIS driver so that it doesn't use STL. However, we (myself or Lichun Wang) will only do so with an explicit request, as this needs some work (I just looked at the ILWIS driver, and STL is used in almost every line of source code).

...

About simply omitting transform / tolower, not sure if this can be done like that: starting at the second occurrence in file ilwisdataset.cpp, it is getting suspicious because at least one such "lowered" string propagates a few function calls. I'd need to fully check the code flow.

Frank Warmerdam wrote:

Bas, I notice the "tolower"ed names are passed on to create new filenames. But I am reasonably convinced that the handling of mixed case filenames is currently quite broken on operating systems with case sensitive filenames. On windows it shouldn't matter. I'd like to make some improvements with regard to case sensitivity in filenames but I think this might exceed the time I can spend today.

[frankie]

Still another issue with gcc 4.3:  http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=462709

[warmerdam]

I have changed include <climits> to include <limits.h> in the hopes of resolving this (r13693). But since I can't reproduce the original problem with gcc 4.3, I can't know off hand if this will fix it. Can you put this in the queue for testing frankie? If it works, I'll push it back into 1.5 branch.

[mloskot]

If I may share my 5 cents,proper fix should include update to ILWIS so it does not mix libraries from C98, C99 and C++ stuff in the same sources. Replacing <climits> to <limits.h> is not a solution because <climits> does define constants like SHRT_MIN. I suppose the ILWIS driver needs some cleanup.

[mloskot]

Replying to mloskot:

If I may share my 5 cents,proper fix should include update to ILWIS so it does not mix libraries from C98, C99 and C++ stuff in the same sources. Replacing <climits> to <limits.h> is not a solution because <climits> does define constants like SHRT_MIN. I suppose the ILWIS driver needs some cleanup.

Sorry, but seems I've missed the point :-) I'm revoking this comment.

[retsios]

This is the MSDN topic about limits.h: "Include the standard header <climits> to effectively include the standard header <limits.h> within the std namespace."

The climits file simply redirects to limits.h

From limits.h the ILWIS driver uses LONG_MIN, SHRT_MAX, LONG_MAX and ULONG_MAX, mostly for deciding how many bytes are needed to store one pixel.

A solution for avoiding #include <climits> would be to copy the corresponding #defines from limits.h

But now I got curious: Is STL support reduced in gcc43? Why do I suddenly see problems with "transform" and "climits" which weren't there before?

[frankie]

Replying to retsios:

This is the MSDN topic about limits.h: "Include the standard header <climits> to effectively include the standard header <limits.h> within the std namespace." The climits file simply redirects to limits.h From limits.h the ILWIS driver uses LONG_MIN, SHRT_MAX, LONG_MAX and ULONG_MAX, mostly for deciding how many bytes are needed to store one pixel. A solution for avoiding #include <climits> would be to copy the corresponding #defines from limits.h But now I got curious: Is STL support reduced in gcc43? Why do I suddenly see problems with "transform" and "climits" which weren't there before?

Because gcc 4.3 does not assume an inmplicit 'using namespace std' as before.

[mloskot]

Replying to retsios:

This is the MSDN topic about limits.h: "Include the standard header <climits> to effectively include the standard header <limits.h> within the std namespace."

This comment from the MSDN is specific to Microsoft. The C++ Standard in chapter 18.2.2 says as follows:

The contents (of the <climits>) are the same as the Standard C library header <limits.h>

but how this is achieved is implementation-specific.

The climits file simply redirects to limits.h

In Visual C++, yes, but GCC can achieve this goal differently. And both compilers/libraries do it correctly. What's important here, is that it's incorrect do depend on the fact that <climits> might include <limits.h>.

From limits.h the ILWIS driver uses LONG_MIN, SHRT_MAX, LONG_MAX and ULONG_MAX, mostly for deciding how many bytes are needed to store one pixel.

These are scope-less macros.

A solution for avoiding #include <climits> would be to copy the corresponding #defines from limits.h

IMHO, it's unnecessary.

But now I got curious: Is STL support reduced in gcc43? Why do I suddenly see problems with "transform" and "climits" which weren't there before?

I think the reason of the problem is that C++ compiler in GCC becomes, from release to release, more strict while compiling C++ code and it refuses small but important missing things like those in the ILWIS driver (missing <string> inclusion, using string type as it would be in global scope, missing <algorithm> header, using algorithm as they are always available by default without including a header, missing <cctype> header that declares tolower/toupper in std:: namespace, etc.)

[retsios]

The lines "using namespace std", #include <string>, #include <algorithm>, and #include <map> are not implicit: they're found in file ilwisdataset.h . I'll even remove the #include <algorithm>, since the calls to STL algorithm "transform" have been eliminated. Regarding <climits> or <limits.h>: feel free to change it if that solves the problem compiling under gcc43.

[mloskot]

Generally, this is incorrect and may lead to troubles to assume that a symbol will be declared because a header is in the include chain, even if a header is not included directly. This is a common practice in C, but in C++ it simply does not work.

In other words: - symbol A - header a.h is included by x.h and this one is included in x.cpp it does not mean A will be auto-magically available in x.cpp.

Correct and portable way is to explicitly include all headers for all symbols that are used in particular translation unit (means, a.h is included in x.h and a.h should be also included in x.cpp).

[retsios]

I'm not convinced on this. Following this rule would even result into .h files included multiple times accidentally/unintentionally. It is safe to assume that if x.cpp includes x.h and x.h includes a.h, then x.cpp includes both a.h and x.h, in the order specified. To compile a .cpp file, the compiler creates one large "virtual" file by recursively inserting all include files into the .cpp file. This is how I have programmed C++ since 1992. If you have reason to believe that this has changed, please point me to an official resource saying so, because it seems I will have to learn C++ again ;)

[mloskot]

Replying to retsios:

I'm not convinced on this. Following this rule would even result into .h files included multiple times accidentally/unintentionally.

No, it would not because headers use inclusion guards.

It is safe to assume that if x.cpp includes x.h and x.h includes a.h, then x.cpp includes both a.h and x.h, in the order specified. [...]

It will include the header (what's defined by the C++ Standard too), but it may cause troubles ie. if in a.h one has opened a namespace then scopes in x.h (so x.cpp) are polluted. In C++, there are user-defined namespaces but in C everything lives in global namespace. If namespaces are improperly opened and global namespaces are polluted, then headers including headers including private-stuff-to-global may cause problems like names clashes or redefinitions. It's hard to predict and remember what's may and what may not be included by headers of upper level. So, it's more safe to assume that nothing is available in a translation unit if a particular header is has not been included.

Also, explicit inclusion of headers in other units helps in maintenance and makes it easier to remove unused features: if you remove functionality from x.cpp that uses a.h and a.h is no longer needed, then it's easier o remove a.h from x.cpp than to walk up file dependencies searching what to remove. ...etc. etc.

I'm not saying about anything that's specified in the Standard, but I'm just suggesting some practice that IMHO makes life easier.

[mloskot]

Just to give an example why I'm talking so (too) much about being explicit :-)

// a.h ////////////////////////////////////
#ifndef A_H
#define A_H
double foo(double x){ return (x < 0 ? (-x) : x); }
#endif

// b.h ////////////////////////////////////
#ifndef B_H
#define B_H
#include "a.h" // XXX: What if someone will remove this line
int foo(int x){ return (x < 0 ? (-x) : x); }
#endif


// test.cpp ////////////////////////////////////

#include "b.h" // XXX: We assume that foo(double) is available
int main()
{
   double x = 1 / foo(0.5);
   return 0;
}

In the example above, if someone will remove a.h from b.h file, then we will encounter run-time errors or undefined behavior (no compile time errors).

IMHO, this one is clearer

// b.h ////////////////////////////////////
#ifndef B_H
#define B_H
int foo(int x){ return (x < 0 ? (-x) : x); }
#endif

// test.cpp ////////////////////////////////////
#include "b.h"
#include "a.h" // XXX: Removing this line mean a maintainer reviewed the test.cpp
int main()
{
    double x = 1 / foo(0.5);
    return 0;
}

Anyway, probably I've moved the discussion too far and got off topic.

[retsios]

If it is for readability, I agree: the rule is simple: "if possible, avoid #include in .h files". I was thinking that you were telling me that gcc4.3 does not support #include in .h anymore.

Anyway, close this topic. Back to the ILWIS driver: does it compile under gcc4.3 now? If not, what's the error message?

Moving the STL #includes from the .h to the .cpp (above #include "ilwisdataset.h") is unwanted, as it would spoil the interface (I don't want to get compile errors by simply including a .h file in my .cpp file). Copying the #includes to the .cpp file is duplication (can't assume a .h file is always protected against multiple inclusion).

Leave the ILWIS driver as it is if there's no compile error after Mr. Warmerdam's last change. Check the rest of GDAL: you'll find plenty #includes in .h files, without duplicate #include in the corresponding .cpp file.

[mloskot]

Replying to retsios:

If it is for readability, I agree: the rule is simple: "if possible, avoid #include in .h files".

I was suggesting the opposite: there is completely no reason to avoid including headers in headers.

I was thinking that you were telling me that gcc4.3 does not support #include in .h anymore.

Sure I wasn't.

[mloskot]

I've just checked and the ILWIS driver compers on Ubuntu 7.04 + GCC 3.4 as well as with GCC 4.1

However, this works only because a buggy #ifdef condition in cpl_string.h:160 which pollutes the global namespace with string symbol. The preprocessor condition should be changed to:

#if defined(_MSC_VER)
#  if (_MSC_VER <= 1202)
#    define MSVC_OLD_STUPID_BEHAVIOUR
#  endif
#endif

Nother place polluting the global scope is in ilwisdataset.h:50. Opening namespaces in headers  should be avoided, so in my opinion the pointed places are candidates to be fixed.

So, I've taken the liberty to fix it and I've attached the patch (ilwis-cleanup-mloskot.patch). I've checked that GDAL compiles with this patch using GCC 3.4, 4.1 and Visual C++ 8.0 (2005).

[mloskot]

Patch cleaning headers/namespaces issue. The C++ headers like <climits> work perfectly well with GCC 3.4 and other compilers (no reason it shouldn't work).

[dron]

I have tested Mateusz's patch with MSVC 6.0 compiler and GCC 4.3 (Debian testing). MSVC works fine, but GCC fails with the following errors:

IdrisiDataset.cpp: In static member function 'static GDALDataset* IdrisiDataset::CreateCopy(const char*, GDALDataset*, int, char**, int (*)(double, const char*, void*), void*)':
IdrisiDataset.cpp:938: error: 'SHRT_MIN' was not declared in this scope
IdrisiDataset.cpp:939: error: 'SHRT_MAX' was not declared in this scope

[retsios]

I checked the namespace peculiarity mentioned. Indeed, many things include "cpl_string.h", therefore STL namespace declaration for std::string is (unintentionally) ensured almost everywhere in GDAL.

However, please don't apply the patch prepending "std::" everywhere in the ILWIS driver, unless it really solves an issue. Nothing from GDAL includes ilwisdataset.h, so the namespace declaration doesn't propagate. And we'd like to keep the ILWIS driver neat as it is now.

[mloskot]

Replying to retsios:

However, please don't apply the patch prepending "std::" everywhere in the ILWIS driver, unless it really solves an issue. Nothing from GDAL includes ilwisdataset.h, so the namespace declaration doesn't propagate. And we'd like to keep the ILWIS driver neat as it is now.

Don't worry, I won't apply this patch on basis of my personal decision. Here, I'm just expressing my personal opinion, but not the official GDAL team statement. I also decided to contribute the patch on my own.

Just to give final summary, My opinion is as follows:

• the cpl_string.h is buggy and it should be fixed

• after it's fixed, the ILWIS as well as IDRISI drivers do not compile cleanly, so they are buggy too

• referring to string vs std::string, I don't understand what's not neat in using regular features of a programming language, but this is not the point here to answer my questions.

• the only thing I'd like to force here is to apply the fix for cpl_string.h

• generally, if we won't use the language correctly and we refuse some minimum of strictness, then upcoming versions of various compilers bringing fixes to their own bugs, will experience our asses hard

Certainly, final decision does not belong to me (and it should not).

[mloskot]

Replying to dron:

I have tested Mateusz's patch with MSVC 6.0 compiler and GCC 4.3 (Debian testing). MSVC works fine, but GCC fails with the following errors:

Andrey, thank you for doing this. I'm installing Debian lenny/sid and will, so I can test it and fix my patch.

[warmerdam]

Apparently the IDRISI driver has the same problem with SHRT_MIN. I have patched cpl_port.h (r13696) to include limits.h in the hopes of avoiding this and similar future problems. Could someone confirm trunk builds on GCC 4.3 and note it here?

Then I'll backport just r13696 into 1.5 branch.

[mloskot]

I'd like to confirm that current trunk successfully builds using GCC 4.3.

I also tested trunk with applied patch ilwis-cleanup-mloskot.patch and it works well too.

Actually, the only allowed way to add C Library features to CPL (gdal/port) is to use C headers (ie. limits.h) but not C++ (ie. climits) because of the fact that the CPL must be compilable using C compiler.

[warmerdam]

I have migrated r13696 into 1.5 branch (r13724) and am closing this ticket.