getDimension() - Possible overflow of unsigned int type

[mloskot]

Dimension class is mainly a manager of enum

enum DimensionType {
   DONTCARE=-3,
   True,
   False,
   P,
   L,
   A
};

Enumerators in the DimensionType are of type of signed int but NOT unsigned.

Problems:

• inconsistency - getDimension() function member in classes of Geometry hierarchy mixing signed int and unsigned int.
• getDimension() in classes CoordinateSequence and CoordinateArraySequence returns unsigned int, so it's consusing as well as potentially danger when someone will want to return less-than-zero value of DimensionType enum, example:

  unsigned int getDimension()
  {
     return -3;
  }
  

[strk@…]

in JTS there's no enum, those values are static int members of the Dimension class.
I guess we could switch (True and False are also
declared as negative values)

[strk@…]

JTS algorithms rely on specific values, also
using negative ones. An example is GeometryCollection::getDimension()
it returns Dimension::False (-1 in JTS) for
an empty collection.

Also, it's often use the 'value' rather then the symbol
in many classes, whereas 0==point, 1==line, 2==area
this leaves only space for False,True and DONT_CARE in higher numbers, and algorithms could break...

Again, we want to match JTS as closely as possible, so if JTS needs the negative values, and enums() don't have them we'd better switch to non-enums.

[strk@…]

Oh...I think I understood the problem now :)
Any 'unsigned int' return from getDimension()
would be a bug, but CoordinateSequence::getDimension
does NOT refer to the Dimension::DimensionType
enum !

CoordinateSequence dimension is the number of ordinates
in space (2d, 3d, ...)

I hope this resolves the whole issue (we keep using enums)

[mateusz@…]

> JTS algorithms rely on specific values,
> also using negative ones.

No problem, C++ enumeration can include negative enumerators.

> Also, it's often use the 'value'
> rather then the symbol
> in many classes,
> whereas 0==point, 1==line, 2==area
> this leaves only space for False,True
> and DONT_CARE in higher numbers,
> and algorithms could break...

As I said above, enumeration can host negative values.
Second, there is no problem to integer value explicitely if required:

enum color { red = -1, green = 0, blue = 1 };

int x = red; // OK

but
color c = 0; // ERROR: type mismatch

Source: C++ Standard, 7.2 Enumeration declarations

> Again, we want to match JTS as closely as possible,
> so if JTS needs the negative values, and enums()
> don't have them we'd better switch to non-enums.

I'm really sorry, but I have to say I don't agree.
Why GEOS is written in C++ if it's a Java code
wearing C++ syntax?

There is NO WAY to solve the same problem in C++ using Java semantic without huge amount of problems.
I'd also say that the Javism in GEOS is one of the main reason of problems, memory leaks and instability.
I'm analysing GEOS code and JTS code in details and I'm trying to understand both, so if it's appropriate, we can discuss it in details on the mailing list.

[mateusz@…]

> Any 'unsigned int' return from getDimension()
> would be a bug, but CoordinateSequence::getDimension
> does NOT refer to the Dimension::DimensionType
enum !

Not would but it will be a bug.

unsigned int foo()
{
   return -10;
}

Here is foo() function will return:
mloskot@dog:~/tmp$ g++ -pedantic x.cpp
x.cpp: In function unsigned int foo():
x.cpp:5: warning: converting negative value -0x0000000000000000a to unsigned int
mloskot@dog:~/tmp$ ./a.out
4294967286
mloskot@dog:~/tmp$


> CoordinateSequence dimension is the number
> of ordinates in space (2d, 3d, ...)
> I hope this resolves the whole issue
> (we keep using enums) 

With enums, you will provide type-safety.
If Java is (was) not type-safe in this case, then GEOS *should not* follow such unsafety. Instead, it's better to stay with Java.

[strk@…]

Ok, so my proposal is:
1) have methods returning values from Dimension enum
   return Dimension::dimensionType as you suggested
2) give all of Dimension::dimensionType explicit values
3) use symbolic names always

Satisfied ?

[mateusz@…]

> Ok, so my proposal is:
> 1) have methods returning values from
> Dimension enum return Dimension::dimensionType as you suggested
> 2) give all of Dimension::dimensionType explicit values
> 3) use symbolic names always

Yup, should work fine.

> Satisfied ?

Hmm, my satisfaction is the point. User's safety and GEOS stability is the point.

[mateusz@…]

> > Satisfied ?
> 
> Hmm, my satisfaction is the point.

Certainly, it should read as:

"my satisfaction is NOT the point"

:-)

[strk@…]

I finally fixed this, hopefully existing unit tests
would have cautgh any error :)

[(none)]

Milestone 3.0.0 deleted