sfd support for r.terraflow

[adanner]

Single Flow Direction (sfd) support in r.terraflow is broken. Even when the -s flag is specified, multiple flow directions are used most of the time. Patch included.

The main purpose of the patch is to support single flow directions

properly (SFD) in r.terraflow. The previous version has a -s switch for SFD, but it is not implemented properly. Tools like Hamish's matlab script for converting terraflow flow direction values to sfd are incomplete and sometimes result in ambiguous because they are based on incomplete cases in the terraflow source. With the new patch, sfd should be supported correctly and the terraflow direction grid should only output 1 of 9 possible direction values (0,1,2,4,8,16,32,64,128), instead of the 256 directions it printed before.

The second purpose of the patch is to eliminate gcc 4.2 warnings of the

form

warning: deprecated conversion from string constant to ‘char*’

This required making two new C files to handle the option parsing in

C-style instead of C++. The changes are minor but touched several files. The actual flow direction fix is rather localized. New files have been indented according to the SUBMITTING rules

users using the default Multiple Flow Direction (mfd) options should

see no changes. Users of sfd should notice that it works.

[adanner]

patch to terraflow to support sfd

[adanner]

new source file to go along with patch

[adanner]

new source file to go along with patch

[adanner]

Can someone double check the Makefile? I wasn't sure about this BROKEN_MAKE flag. The modifications to the Makefile are just to build the new C source for tflowopts.c, so if the BROKEN_MAKE flag is C++/r.terraflow specific, I don't think there is a problem.

[glynn]

Replying to adanner:

Can someone double check the Makefile? I wasn't sure about this BROKEN_MAKE flag. The modifications to the Makefile are just to build the new C source for tflowopts.c, so if the BROKEN_MAKE flag is C++/r.terraflow specific, I don't think there is a problem.

BROKEN_MAKE is only required for rules with order-only dependencies (some versions of make don't handle these correctly).

However: is it really necessary for the new file to be in C instead of C++? It would simplify matters to keep everything in the same language.

[adanner]

Replying to glynn:

Replying to adanner:

Can someone double check the Makefile? I wasn't sure about this BROKEN_MAKE flag. The modifications to the Makefile are just to build the new C source for tflowopts.c, so if the BROKEN_MAKE flag is C++/r.terraflow specific, I don't think there is a problem.

BROKEN_MAKE is only required for rules with order-only dependencies (some versions of make don't handle these correctly).

However: is it really necessary for the new file to be in C instead of C++? It would simplify matters to keep everything in the same language.

Yes, the new file must be in C and not C++. The g++-4.2 throws a warning if you try to assign a string constant to a char *. E.g.,

cat test.c int main(){

char * example = "hello";

}

gcc test.c -o test ok

mv test.c test.cc; g++ test.c -o test test.cc: In function ‘int main()’: test.cc:2: warning: deprecated conversion from string constant to ‘char*’

In C++, the following are preferred and do not give a warning

std::string example="hello"; const char * example2="hello";

However, as the code in question is initializing a bunch of GRASS structs which cannot use const or std::string, it is best to do this in C and link the rest of terraflow to this C code.

[glynn]

Replying to adanner:

However: is it really necessary for the new file to be in C instead of C++? It would simplify matters to keep everything in the same language.

Yes, the new file must be in C and not C++. The g++-4.2 throws a warning if you try to assign a string constant to a char *. E.g.,

Sorry; I didn't notice that you had already mentioned that in the original report.

Personally, I wouldn't be especially concerned about the warnings. Especially as they are quite legitimate; those fields should really be "const char *", as they normally get assigned from string literals. Although string literals are treated as "char *", on any platform we care about, you will get a segfault if you try to modify them.

The only fields which are likely to be modified are answer and answers, and even that's dubious in terms of clarity. Code should ideally copy the values to a separate buffer rather than modifying them in-place.

Moving the code from C++ to C is simply hiding a legitimate issue. The compiled code will be essentially the same regardless of language, and the problems are the same. It's just that, for historical reasons, C doesn't complain while C++ does.

My attitude to warnings is to either fix them for real or leave them there, with "hiding" them being the worst choice.

[adanner]

Replying to glynn:

Personally, I wouldn't be especially concerned about the warnings. Especially as they are quite legitimate; those fields should really be "const char *", as they normally get assigned from string literals. Although string literals are treated as "char *", on any platform we care about, you will get a segfault if you try to modify them.

Moving the code from C++ to C is simply hiding a legitimate issue. The compiled code will be essentially the same regardless of language, and the problems are the same. It's just that, for historical reasons, C doesn't complain while C++ does.

My attitude to warnings is to either fix them for real or leave them there, with "hiding" them being the worst choice.

I did fix a few C++ warnings correctly in the C++ code using either std::string or appropriate use of const. These changes were in IOStream/include/mm_utils.h IOStream/include/mem_stream.h, IOStream/include/ami_stream.h, and the associated .cc files. The only C-like fixes is to hide warnings from setting fields in the Grass struct GModule and struct Option. Every other module has similar code, but since every other module is in C, there are no warnings. I'm not hiding anything terrafow related in this .c file.

If you want, I can throw the code from tflowopt.c back into main.cc and then you'll get 30+ warning about how terraflow is trying to connect to the rest of Grass. This seems excessive, since there currently is no right way to fix a C-style interface in C++ without bloating the code and making it unreadable. Basically every option parameter would need to be set as follows.

string tmp="memory";

mem->key=tmp.c_str();

tmp="300"

mem->answer=tmp.c_str();

Blech. I personally think the .c file is the right way to have C++ code talk to the few C-style structs it needs to initialize parameters.

I'd happily push the code back into main.cc, but it would be nice to get a patch committed. I've received email about this error at least three times over the past few years. Just let me know if you want me to revert the tflowopts changes.

[glynn]

Replying to adanner:

If you want, I can throw the code from tflowopt.c back into main.cc and then you'll get 30+ warning about how terraflow is trying to connect to the rest of Grass.

That is my preference.

This seems excessive, since there currently is no right way to fix a C-style interface in C++ without bloating the code and making it unreadable.

Correct. Which is why the interface needs to be fixed in C; i.e. add the "const" to the fields of "struct Option" in gis.h. Having the compiler remind people about this on a regular basis is the best way to make it happen.

Actually, I'll look into that side of it now.

[glynn]

Replying to glynn:

Correct. Which is why the interface needs to be fixed in C; i.e. add the "const" to the fields of "struct Option" in gis.h. Having the compiler remind people about this on a regular basis is the best way to make it happen.

Actually, I'll look into that side of it now.

Done. All of the fields of the Option, Flag and Module structures which might reasonably be initialised from string literals are now declared "const".

[adanner]

Replying to glynn:

Replying to adanner:

If you want, I can throw the code from tflowopt.c back into main.cc and then you'll get 30+ warning about how terraflow is trying to connect to the rest of Grass.

That is my preference.

new patch uploaded. Patch is now against svn-trunk, but the changes can be merged back to 6.4.0. I updated the milestone and version tags.

This seems excessive, since there currently is no right way to fix a C-style interface in C++ without bloating the code and making it unreadable.

Correct. Which is why the interface needs to be fixed in C; i.e. add the "const" to the fields of "struct Option" in gis.h. Having the compiler remind people about this on a regular basis is the best way to make it happen.

Actually, I'll look into that side of it now.

Thanks, that eliminates many of the warnings. There are only four left on options->answer, but I'm content with only four warnings. Fixing those last four require some tedious buffer creation/copying.

[glynn]

Replying to adanner:

Correct. Which is why the interface needs to be fixed in C; i.e. add the "const" to the fields of "struct Option" in gis.h. Having the compiler remind people about this on a regular basis is the best way to make it happen.

Actually, I'll look into that side of it now.

Thanks, that eliminates many of the warnings. There are only four left on options->answer, but I'm content with only four warnings. Fixing those last four require some tedious buffer creation/copying.

It should suffice to use:

opt->answer = G_store("literal");

The current interface permits modules to modify the ->answer field, and some do.

Hmm. Actually, changing the "answer" field only causes compilation failures for 3 modules (d.linegraph, i.atcorr and r.terraflow). d.linegraph does actually modify the value in place, but that isn't hard to fix. The other two only complain because they're in C++.

However, there are quite a few "incompatible pointer type" warnings. I suspect that most of those are just because someone simply forgot to add the "const" to a parameter declaration, but checking for any cases where it does actually matter could take a while.

The hardest ones to deal with are where pointers are passed to the DBMI functions (directly or through the vector functions), as hardly any char* parameters have the "const" qualifier.

[adanner]

new patch against grass_trunk that eliminates c files in previous patch

[adanner]

Replying to glynn:

Replying to adanner:

Correct. Which is why the interface needs to be fixed in C; i.e. add the "const" to the fields of "struct Option" in gis.h. Having the compiler remind people about this on a regular basis is the best way to make it happen.

Actually, I'll look into that side of it now.

Thanks, that eliminates many of the warnings. There are only four left on options->answer, but I'm content with only four warnings. Fixing those last four require some tedious buffer creation/copying.

It should suffice to use:

opt->answer = G_store("literal");

Another rounds of updates to the patch to use G_store. It works great for r.terraflow which now compiles against gcc 4.2.3 with zero warnings. Thanks for your help.

[glynn]

Replying to adanner:

Another rounds of updates to the patch to use G_store. It works great for r.terraflow which now compiles against gcc 4.2.3 with zero warnings. Thanks for your help.

Patch committed.

[neteler]

Patch backported to 6.4.svn (code is identical) but it fails there with:

c++ -DELEV_FLOAT -L/home/neteler/grass64/dist.x86_64-unknown-linux-gnu/lib -Wl,--export-dynamic  -L/usr/lib64 -Wl,-rpath-link,/home/neteler/grass64/dist.x86_64-unknown-linux-gnu/lib -L/usr/lib64  -o /home/neteler/grass64/dist.x86_64-unknown-linux-gnu/bin/r.terraflow  OBJ.x86_64-unknown-linux-gnu/FLOAT/main.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/common.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/stats.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/fill.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/types.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/ccforest.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/nodata.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/plateau.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/direction.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/water.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/filldepr.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/grid.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/genericWindow.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/flow.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/sweep.o  OBJ.x86_64-unknown-linux-gnu/FLOAT/weightWindow.o -lgrass_gis -lgrass_datetime -lz      -lm  \
        -lz  -liostream
OBJ.x86_64-unknown-linux-gnu/FLOAT/main.o: In function `AMI_STREAM':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/fill.o: In function `AMI_STREAM':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/fill.o:/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: more undefined references to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)' follow
OBJ.x86_64-unknown-linux-gnu/FLOAT/ccforest.o: In function `im_buffer':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/imbuffer.h:84: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/ccforest.o: In function `em_buffer':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:440: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:449: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:455: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:462: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/ccforest.o:/home/neteler/grass64/raster/r.terraflow/IOStream/include/empq_impl.h:1351: more undefined references to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)' follow
OBJ.x86_64-unknown-linux-gnu/FLOAT/nodata.o: In function `AMI_STREAM':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/plateau.o: In function `AMI_STREAM':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/water.o: In function `AMI_STREAM':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/water.o: In function `im_buffer':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/imbuffer.h:84: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/water.o: In function `em_buffer':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:440: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:449: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:455: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:462: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/water.o:/home/neteler/grass64/raster/r.terraflow/IOStream/include/empq_impl.h:1351: more undefined references to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)' follow
OBJ.x86_64-unknown-linux-gnu/FLOAT/flow.o: In function `AMI_STREAM':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/sweep.o: In function `AMI_STREAM':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/ami_stream.h:203: undefined reference to `ami_single_temp_name(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char*)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/sweep.o: In function `im_buffer':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/imbuffer.h:84: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/sweep.o: In function `em_buffer':
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:440: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:449: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:455: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
/home/neteler/grass64/raster/r.terraflow/IOStream/include/embuffer.h:462: undefined reference to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
OBJ.x86_64-unknown-linux-gnu/FLOAT/sweep.o:/home/neteler/grass64/raster/r.terraflow/IOStream/include/empq_impl.h:437: more undefined references to `MEMORY_LOG(std::basic_string<char, std::char_traits<char>, std::allocator<char> >)' follow
collect2: ld returned 1 exit status
make: *** [/home/neteler/grass64/dist.x86_64-unknown-linux-gnu/bin/r.terraflow] Error 1


[neteler@markus ~]$ uname -a
Linux markus 2.6.22.18-laptop-1mdv #1 SMP Mon Feb 11 13:53:01 EST 2008 x86_64 Intel(R) Core(TM)2 CPU         T5500  @ 1.66GHz GNU/Linux

[neteler@markus ~]$ gcc -v
Using built-in specs.
Target: x86_64-mandriva-linux-gnu
...
gcc version 4.2.2 20071128 (prerelease) (4.2.2-3.1mdv2008.0)

Markus

[adanner]

Replying to neteler:

Patch backported to 6.4.svn (code is identical) but it fails there with:

You may have an old copy of libiostream.a sticking around. I found that when I ran make clean in the r.terraflow directory, the libiostream.a file didn't get destroyed and didn't get rebuilt on make, causing similar linking errors.

try rm ../../dist.<DISTARCH>/lib/libiostream.a from the r.terraflow directory and see if it builds. I can build in 6.4 with the latest updates. The patch was originally against 6.4. It should work. I'm on i686-pc-linux-gnu.

The makefile should probably be modified to remove libiostream.a on make clean, but I hate messing with makefiles, so I'll leave that to someone else.

[neteler]

Andrew, excellent.

rm -f ../../dist.x86_64-unknown-linux-gnu/lib/libiostream.a

did the job (so some Makefule magic might be needed to fix this).

Could you please take a look at description.html if it needs an update for the new functionality?

thanks, Markus

[adanner]

Replying to neteler:

Andrew, excellent.

rm -f ../../dist.x86_64-unknown-linux-gnu/lib/libiostream.a

did the job (so some Makefule magic might be needed to fix this).

Could you please take a look at description.html if it needs an update for the new functionality?

thanks, Markus

No update needed. The "new" functionality is to get r.terraflow to do what it says in description.html. Are the images in description.html based on spearfish? I might need to update the image for sfd (mfd is unchanged), but I can't get to that until next week. Laura may want to update her contact info. The Duke link is 404ed now. I'll follow up with her.

[glynn]

Replying to adanner:

The makefile should probably be modified to remove libiostream.a on make clean, but I hate messing with makefiles, so I'll leave that to someone else.

"make clean" should only remove intermediate files; it shouldn't touch anything in the dist.<arch> directory.

If you want to force libiostream.a to be remade before making r.terraflow, mark $(IOSTREAM_DEPLIB) as a .PHONY target, so that it is re-made regardless of whether or not it already exists.

The same issue applies to every other library in GRASS. However, having libiostream in the r.terraflow directory seems to lead people to assume that it will be re-made automatically when making r.terraflow.

For other libraries, most people would be aware that the library needs to be explicitly re-made before anything which depends upon it. This problem should go away if libiostream is moved to the lib directory.

[adanner]

contact updates, typo fixes for description.html

[adanner]

Replying to adanner:

Replying to neteler:

Could you please take a look at description.html if it needs an update for the new functionality?

thanks, Markus

No update needed. The "new" functionality is to get r.terraflow to do what it says in description.html. Are the images in description.html based on spearfish? I might need to update the image for sfd (mfd is unchanged), but I can't get to that until next week. Laura may want to update her contact info. The Duke link is 404ed now. I'll follow up with her.

a small patch to description.html is attached to the ticket to update contact info.

[neteler]

Thanks, docs patches applied.

Markus

[mlennert]

Any reason this bug is still open ? Seems fixed from the comments.

[neteler]

Looks fine, closing.