Add test framework, C++11 compilation mode and warning fixes #1021

rouault · 2018-05-29T22:14:28Z

3 somewhat related topics:

Fix warnings found by clang with new warning flags to be added in later commit
Fixes consist in:
- no use of comma operator for multi statement purpose
- avoid confusing comma in for loops first and third clauses
- addition of explicit long to int casts
Enable c++11 compilation with autotools and cmake, hardened compilation flags and add catch2 framework. Hardened compilation flags come from GDAL' configure.ac and are 'combat proven'. Dummy test added just to demonstrate catch2 based tests work.
CI environment: adapt for C++11 capable environments

kbevers

I'll give this a more thorough look tomorrow. For now I have once immediate comment:

addition of explicit long to int casts

This just seem to be screwing with what was done in #949. It may be that the type conversions can be done safer with a C++ compiler, but if that is the case this should be reverted more carefully to what it was before #949. There's going to be a bunch of unnecessary floor()s and lround()s otherwise.

rouault · 2018-05-29T23:07:51Z

. It may be that the type conversions can be done safer with a C++ compiler

Not related at all with C++. .c files are still compiled with the C compiler. This is with the C compiler of clang with the -Wshorten-64-to-32 warning that warns when a long is implicitly cast to int. Basically, the cast makes it more obvious that we may have a truncation issue.

rouault · 2018-05-29T23:28:17Z

I've revised the 'int x = (int)long_variable' casts to rather promote the storage type to long when possible, and when not practical adding explicit bound checking

mloskot · 2018-05-30T09:29:48Z

test/Makefile.am

@@ -1,3 +1,3 @@
-SUBDIRS = gie gigs
+SUBDIRS = gie gigs cpp


test/cpp - this is an unfortunate name as too generic, assuming the tests may be re-organized at some point, as @kbevers suggested on the list.

The structure could aim for something like

tests/gigs tests/gie tests/unit tests/functional - might or might not be needed, as per the recent discussion on list tests/apps ...

OK see 6fbfa8e

I wished @kbevers commented on my comment first in case he disagrees, but LGMT, thanks!

@mloskot I think your suggested structure looks good

kbevers

Looks good to me. I've only got a few minor suggestions regarding the code changes to avoid warnings.

You've put the catch header in test/cpp/. Does that imply that we will be putting all catch tests in that directory? Or did it just go there for practical reasons? I am just thinking if it would be better to put it a level higher so we can have something like test/cpp/, test/apps/, test/whatever/ giving a bit of structure to the test framework while lowering the depth of the file tree. Just a thought.

kbevers · 2018-05-30T08:58:47Z

src/dmstor.c

@@ -34,15 +34,19 @@ dmstor_ctx(projCtx ctx, const char *is, char **rs) {
 		*rs = (char *)is;
 	/* copy sting into work space */
 	while (isspace(sign = *is)) ++is;
-	for (n = MAX_WORK, s = work, p = (char *)is; isgraph(*p) && --n ; )
+        n = MAX_WORK;


These lines are indented a level too much. Makes them look like they belong to the while loop.

kbevers · 2018-05-30T09:00:09Z

src/gen_cheb.c

@@ -16,7 +16,9 @@ extern void p_series(Tseries *, FILE *, char *);

 void gen_cheb(int inverse, projUV (*proj)(projUV), char *s, PJ *P,
              int iargc, char **iargv) {
-	int NU = 15, NV = 15, res = -1, errin = 0, pwr;
+	long NU = 15, NV = 15;
+        int errin = 0, pwr;


Indentation. This line and below.

kbevers · 2018-05-30T09:11:02Z

src/optargpm.h

@@ -525,28 +525,40 @@ OPTARGS *opt_parse (int argc, char **argv, const char *flags, const char *keys,
                    *equals = 0;
                c = opt_ordinal (o, crepr);
                if (0==c)
-                    return fprintf (stderr, "Invalid option \"%s\"\n", crepr), (OPTARGS *) 0;
+                {


This is nit-picking, but since the rest of the code in this file has starting brackets at the same line as the statement I think that should be done here for consistency as well. Also, this is how clang-format would do it with the default llvm setup.

kbevers · 2018-05-30T09:19:14Z

src/proj_etmerc.c

+    p = p1 + len_p1;
+    h1 = *--p;
+    for (; p - p1; )
+    {


Similar to comment above. Bracket on same line as statement.

…er commit Fixes consist in: - no use of comma operator for multi statement purpose - avoid confusing comma in for loops first and third clauses - avoid implicit long to int casts by storing to long, or explicit bound checking before cast

… flags and add catch2 framework Hardened compilation flags come from GDAL' configure.ac and are 'combat proven' Dummy test added just to demonstrate catch2 based tests work.

…ar case (detected by gcc 8.1)

kbevers · 2018-05-30T10:37:23Z

Do you mind waiting until 5.1.0 is released to merge this? I'd rather not complicate the release process more than necessary.

rouault · 2018-05-30T10:56:05Z

Do you mind waiting until 5.1.0 is released to merge this?

Fine for me. An alternate way of proceeding would be to create a release/5.1 branch from master as soon as we want to 'freeze' it regarding new features and issue releases from it. That's typically how I proceed for GDAL releases, but that can involve some cherry-picking between the release and master branch if there are bugfixes. Anyway, that's not critical for me.

Oh, by the way, I just got my first green build on my WIP branch: https://travis-ci.org/rouault/proj.4/builds/385625895

kbevers · 2018-05-30T12:36:21Z

An alternate way of proceeding would be to create a release/5.1 branch from master as soon as we want to 'freeze' it regarding new features and issue releases from it.

I was trying to avoid that but the PR's are piling up already so maybe I'll have to do it anyway. Maybe if I can find the time later today I'll do.

Oh, by the way, I just got my first green build on my WIP branch: travis-ci.org/rouault/proj.4/builds/385625895

Awesome!

rouault · 2018-05-30T12:40:38Z

It might be good anyway to create a "release/5.1" branch (or perhaps "release/5" ?) before merging this PR, so that we can issue bugfix releases for the 5.x stuff if needed

kbevers · 2018-05-30T13:05:47Z

That was the plan, yes. It's just going to be "5.1", keeping in line with the existing branches with the same purpose.

kbevers · 2018-05-30T18:37:30Z

I've created the 5.1 branch so merging won't interrupt my release preparations any more.

Contributors eager to migrate existing tests to Catch2 can get right on it :-)

kbevers requested changes May 29, 2018

View reviewed changes

rouault force-pushed the add_test_framework branch from 472ed39 to ea8098d Compare May 29, 2018 23:02

rouault force-pushed the add_test_framework branch from ea8098d to ea49d58 Compare May 29, 2018 23:27

rouault force-pushed the add_test_framework branch 3 times, most recently from f9e4616 to 9f08e49 Compare May 30, 2018 00:14

mloskot reviewed May 30, 2018

View reviewed changes

kbevers reviewed May 30, 2018

View reviewed changes

rouault added 5 commits May 30, 2018 11:48

Enable c++ compilation with autotools and cmake, hardened compilation…

2905176

… flags and add catch2 framework Hardened compilation flags come from GDAL' configure.ac and are 'combat proven' Dummy test added just to demonstrate catch2 based tests work.

CI environment: adapt for C++11 capable environments

9b82aef

Replace suspicious use of strncat(), despite correct in that particul…

a3c5928

…ar case (detected by gcc 8.1)

Move catch.hpp header in test/; rename test/cpp as test/unit

6fbfa8e

rouault force-pushed the add_test_framework branch from 26e59cb to 6fbfa8e Compare May 30, 2018 10:06

kbevers approved these changes May 30, 2018

View reviewed changes

kbevers merged commit 559e8f1 into OSGeo:master May 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test framework, C++11 compilation mode and warning fixes #1021

Add test framework, C++11 compilation mode and warning fixes #1021

rouault commented May 29, 2018 •

edited

Loading

kbevers left a comment

rouault commented May 29, 2018

rouault commented May 29, 2018

mloskot May 30, 2018 •

edited

Loading

rouault May 30, 2018

mloskot May 30, 2018

kbevers May 30, 2018

kbevers left a comment

kbevers May 30, 2018

rouault May 30, 2018

kbevers May 30, 2018

rouault May 30, 2018

kbevers May 30, 2018

rouault May 30, 2018

kbevers May 30, 2018

rouault May 30, 2018

kbevers commented May 30, 2018

rouault commented May 30, 2018

kbevers commented May 30, 2018

rouault commented May 30, 2018

kbevers commented May 30, 2018

kbevers commented May 30, 2018

Add test framework, C++11 compilation mode and warning fixes #1021

Add test framework, C++11 compilation mode and warning fixes #1021

Conversation

rouault commented May 29, 2018 • edited Loading

kbevers left a comment

Choose a reason for hiding this comment

rouault commented May 29, 2018

rouault commented May 29, 2018

mloskot May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbevers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbevers commented May 30, 2018

rouault commented May 30, 2018

kbevers commented May 30, 2018

rouault commented May 30, 2018

kbevers commented May 30, 2018

kbevers commented May 30, 2018

rouault commented May 29, 2018 •

edited

Loading

mloskot May 30, 2018 •

edited

Loading