Issue2826: BOV support for 2D grids, and add validation for negative grid/brick sizes

[sgpearse]

Fixes #2826 with 2D grids at DCBOV.cpp:290.

This also adds validation for negative values passed to DATA_SIZE and BRICK_SIZE.

Note - this required changing the types for those values from size_t to int. This is because stringstream is used for converting the user's string values to numeric values. Unfortunately, stringstream does not fail or report an error when converting negative values to size_t. Therefore, I had to convert to int, and check for negative values during validation. I don't think this is a problem since grid sizes shouldn't hit INT_MAX any time soon.

[shaomeng]

Note - this required changing the types for those values from size_t to int. This is because stringstream is used for converting the user's string values to numeric values. Unfortunately, stringstream does not fail or report an error when converting negative values to size_t. Therefore, I had to convert to int, and check for negative values during validation. I don't think this is a problem since grid sizes shouldn't hit INT_MAX any time soon.

Scott, I agree that value on a single dimension is unlikely to exceed INT_MAX, but the total number of values is pretty easily to exceed INT_MAX. In other words, the following code will result in int overflows:

int dim_x = userInput(); 
int dim_y = userInput(); 
int dim_z = userInput(); 
size_t last_pencil_end = dim_x * dim_y * dim_z; // int overflow can easily happen

Does it make sense that there's a real danger? Happy to explain in more detail if not.

So, you should store dimension values in size_t type as soon as stringstream gives you a number, and you validated that the number is non-negative. Subsequently, you perform any calculations in size_t type to avoid the scenario demonstrated above. Happy to discuss if you have questions.

[clyne]

This PR does not address the problem of failing to support 2D grids. For better or worse VAPOR treats variables with different topological dimensions as different animals. E.g. 3D variables can be volume rendered, and sliced, etc. 2D variables can be used as height fields, etc. A grid with a dimension length of one has a degenerate dimension that should be removed. I.e. a 10x10x1 grid should be 10x10. Moreover, a grid with dimensions 10x1x10 is also a 10x10 grid. You could argue that the issue of degenerate dimensions should be handled elsewhere in VAPOR, but at the moment it is not. Thus for the BOV reader to support 2D variables it should remove any dimensions of length 1. Otherwise a variable defined as having a grid of 128x128x1, for example, will be treated as a 3D variable and you won't be able to contour it, use it as a height field, etc.

I would be fine deferring action on this and evaluating where the best place is to handle degenerate grid dimensions.

Re supporting (preventing) negative dimension lengths: this approach doesn't make sense to me. A grid can't have a negative length. It's meaningless. You've changed all of the internal representation, and even the public API, to use ints to represent a quantity that can't be negative. Why not simply modify parseHeader to throw an error for negative values?

[sgpearse]

@shaomeng thanks for the chat earlier. I've made the changes we discussed and stepped through the parser to verify that values are being read as integers in _tmpGridSize, then get cast to size_t in the official _gridSize variable (after being validated).

Re supporting (preventing) negative dimension lengths: this approach doesn't make sense to me. A grid can't have a negative length. It's meaningless. You've changed all of the internal representation, and even the public API, to use ints to represent a quantity that can't be negative. Why not simply modify parseHeader to throw an error for negative values?

@clyne The variables _tmpGridSize and _gridSize were previously both size_t. The problem arose when one of the dimension lengths was negative (-10 10 10). I am using std::stringstream to convert strings into numeric values. Unfortunately std::stringstream doesn't throw an error when converting the string "-10" to size_t. It just assigns a huge positive number that can't be validated, because huge positive numbers are still considered valid.

The approach in the recent push reads the DATA_SIZE values as integers and stores them in _tmpGridSize. Then during validation, we check if all values > 0. If so, we cast those integer values into the official copy, std::array<size_t,3> _gridSize.

[clyne]

@sgpearse if you really want to detect negative values for grid dimension than simply format the string as an int (in a local variable), perform your check, and then copy to _tmpGridSize.

vector <int> t;
rc = _findToken(GRID_SIZE_TOKEN, line, t);
.
.
.
if (std::any_of(t.cbegin(), t.cend(), [](int i){ return i < 0; })) {
    return(error)
}
copy_int_to_size_t(t, _tmpGridSize);

[sgpearse]

@sgpearse will throw an error on 2D (10x10x1, etc) grids.

[sgpearse]

The current HEAD of this branch now does not use the implicit cast when doing comparisons. Additionally, we now throw errors when the DATA_SIZE token does not have all values > 1.