Fix EOF detection in p4est_connectivity_getline_upper on Windows

[sloede]

Fix EOF detection in p4est_connectivity_getline_upper on Windows

We have noted that p4est_connectivity_getline_upper sometimes fails on Windows (not always - it seems to depend on the actual C library version used). The problem is rooted in the following lines:

p4est/src/p4est_connectivity.c

Lines 4598 to 4603 in 3a78fd2

	c = fgetc (stream);
	c = toupper (c);
	if (c == EOF && linep == line) {
	P4EST_FREE (linep);
	return NULL;
	}

After reading the next character from the stream with fgetc, it is converted to uppercase using toupper and then checked for the EOF (end of file) marker. This is OK on Linux (and presumably macOS), since

If c is a lowercase letter, toupper() returns its uppercase equivalent, if an uppercase representation exists in the current locale. Otherwise, it returns c.

(ref). Thus, if c is actually EOF (typically represented as -1), there is no uppercase representation and it remains unchanged.

However, on Windows the documentation says

In order for toupper to give the expected results, __isascii and islower must both return nonzero.

Thus, implicitly it says: If it is not an ASCII character, anything can happen! And indeed: In some cases

toupper(-1) == -1 == EOF

but in other cases (in my case when invoking libp4est from Julia),

toupper(-1) == 159 != EOF

and thus the reading loop in p4est_connectivity_getline_upper never terminates.

Proposed changes:
Move the conversion toupper to after the check for EOC, which will make this work no matter on which platform this is running.

[cburstedde]

Thanks for checking this; can I ask you to include a doc/author_schlottke.txt file like the others? -- Ah it's in the other PR.

[sloede]

Sure! Done in 6fe8950.

[cburstedde]

Thanks! I'll merge into master here, usually we ask for the develop branch.
Looking forward to more testing on Windows. :)

[sloede]

usually we ask for the develop branch

Oops, sorry. I'll try to remember next time!

[cburstedde]

No worries and thanks again!

[cburstedde]

> usually we ask for the develop branch Oops, sorry. I'll try to remember next time!

Speaking of next time, how hard would it be to incorporate the changes for Windows from the install document into the code using proper #ifdefs? Note that the CI on the prev3-develop branch already runs on Windows, so I'm currently not sure which of those changes are necessary when.

[sloede]

Speaking of next time, how hard would it be to incorporate the changes for
Windows from the install document into the code using proper #ifdefs?

I am not sure, to be honest, since in my experience there does not exist "the" Windows toolchain (as opposed to, say, Linux/macOS). Most Windows-native projects I've seen (admittedly not that many) seem to rely on MS Visual Studio compilers (cl, or clang-cl). Then there are cross-compilation setups such as BinaryBuilder, which IIR run on AlpineLinux and use either a clang or gcc-based toolchain. There's also Windows + mingw-w64, which I have used to debug stuff on Windows. The GitHub Action toolchain for windows-latest seems to offer a mixture of these options.

Each toolchain seems to have different requirements (I used BinaryBuilder and mingw with gcc). Going through the current examples in https://github.com/cburstedde/p4est/blob/master/INSTALL_WINDOWS, I have had the following experience:

• I never had to use //#include <libgen.h> nor #define __attribute__(x)
• #define srandom srand/#define random rand were only necessary when compiling on Windows + mingw, but not when cross-compiling.
• #define htonl(_val)... is also necessary when cross-compiling (see also the build script for when we auto-build p4est for use in Julia: https://github.com/JuliaPackaging/Yggdrasil/blob/bc8b3f7d15d5a057c4245543148fbec877687bcf/P/P4est/build_tarballs.jl#L34-L50).
• In addition, we need to add -no-undefined on Windows, but only during the linking step (it will cause errors when used during compilation).

While I would really appreciate having these "fixes" be applied automatically on Windows, I am not sure how to go about it such that it works for everyone. I guess one could probably figure this out during CMake preprocessing, but this will have to wait until you retire automake in favor of CMake builds, I guess.

[cburstedde]

> Speaking of next time, how hard would it be to incorporate the changes for Windows from the install document into the code using proper #ifdefs? I am not sure, to be honest, since in my experience there does not exist "the" Windows toolchain (as opposed to, say, Linux/macOS). Most Windows-native projects I've seen (admittedly not that many) seem to rely on MS Visual Studio compilers (`cl`, or `clang-cl`). Then there are cross-compilation setups such as [BinaryBuilder](https://binarybuilder.org/), which IIR run on AlpineLinux and use either a clang or gcc-based toolchain. There's also Windows + [mingw-w64](http://mingw-w64.org/doku.php), which I have used to debug stuff on Windows. The GitHub Action toolchain for `windows-latest` seems to offer a mixture of these options. Each toolchain seems to have different requirements (I used BinaryBuilder and mingw with gcc). Going through the current examples in https://github.com/cburstedde/p4est/blob/master/INSTALL_WINDOWS, I have had the following experience: * I never had to use `//#include <libgen.h>` nor `#define __attribute__(x)` * `#define srandom srand`/`#define random rand` were only necessary when compiling on Windows + mingw, but not when cross-compiling. * `#define htonl(_val)...` is also necessary when cross-compiling (see also the build script for when we auto-build p4est for use in Julia: https://github.com/JuliaPackaging/Yggdrasil/blob/bc8b3f7d15d5a057c4245543148fbec877687bcf/P/P4est/build_tarballs.jl#L34-L50). * In addition, we need to add `-no-undefined` on Windows, but **only** during the linking step (it will cause errors when used during compilation). While I would really appreciate having these "fixes" be applied automatically on Windows, I am not sure how to go about it such that it works for everyone. I guess one could probably figure this out during CMake preprocessing, but this will have to wait until you retire automake in favor of CMake builds, I guess.

Thanks for the elaboration! We'll never retire autotools, ideally both autotools and cmake will work perfectly. Right now #103 contains a decent list of things to improve/add before we can officially support cmake. Putting checks for the items you list into confifgure may not be too hard, actually, thanks! With your pointers I might be doing that on the side, unless someone jumps at it first. I personally wouldn't get to testing it though.

[sloede]

I personally wouldn't get to testing it though.

If it passes Windows CI here, I can give it a shot on my personal Windows + mingw-w64 installation and/or using BinaryBuilder (for cross-compilation).

[cburstedde]

We don't have an autotools/mingw-based CI setup here, yet... but any testing is appreciated.

Right now playing with the feature-random branch to sort some of the above issues.