Fix compiler warnings, part 1 by nilason · Pull Request #1248 · OSGeo/grass

nilason · 2021-01-13T16:22:24Z

Fixes compiler warnings:

-Wabsolute-value
-Warray-bounds
-Wc++11-compat-deprecated-writable-strings
-Wdangling-else

First part addressing #1247.

There are a couple of ambivalent cases and I'm not sure how to correctly address them.
I hope for input from experienced C programmers and leave this open as a draft.

Update:

Modules / code parts directly affected:

imagery/i.aster.toar (partly rewritten)
imagery/i.evapo.time (partly rewritten)
lib/gmath
lib/imagery
lib/vector/neta
raster/r.grow.distance
raster/r.latlong
raster/r.stream.extract (commented out a part)
raster/r.surf.idw
raster/r.viewshed
vector/v.generalize
vector/v.label.sa
vector/v.patch

raster/r.stream.extract/streams.c

lib/raster3d/test/test_put_get_value_large_file.c

raster/r.viewshed/main.cpp

imagery/i.aster.toar/main.c

nilason

Updated imagery/i.aster.toar/main.c according to @metzm suggestion, hopefully I got it right.

raster/r.viewshed/main.cpp

nilason · 2021-01-18T18:09:10Z

This is now ready for review. Please pay special attention to i.aster.toar, which has been partly re-written by @metzm and r.stream.extract where a part of code has been commented out (per suggestion by @metzm).

imagery/i.aster.toar/main.c

imagery/i.evapo.time/main.c

raster/r.viewshed/main.cpp

metzm · 2021-01-21T22:12:44Z

Looks good to me, ready to merge.

There are probably some other modules with wrong array indexing, but I recommend to put these issues in a separate PR if needed.

nilason · 2021-01-22T11:03:35Z

Looks good to me, ready to merge.

Thanks a lot!

I did rearrange the commits: separated the fixes for i.aster.toar and i.evapo.time to one commit each.

There are probably some other modules with wrong array indexing, but I recommend to put these issues in a separate PR if needed.

At the moment these two are the only places reported by clang.

metzm

Interesting how many occurrences of abs() needed to be replaced with fabs()!
Thanks for your work, IMHO now ready to merge.

nilason · 2021-01-29T19:52:21Z

If there are no objections, I will take the liberty to merge in the next couple of days.

wenzeslaus

+1, but I'm also wondering about that commented out code in stream extract. I don't see anything useful in r.watershed blame.

I think, if it is just wrong, it should be deleted with a proper commit message (it is in the history after all if needed). If it is servers as a reminder of something, a comment should be added saying why it is there instead of leaving the reader wondering.

Commenting out all code would indeed fix all or most of compiler warnings, but that would be certainly an unexpected thing to find behind a "fix warnings commit". 😄

nilason · 2021-01-30T11:29:28Z

I think, if it is just wrong, it should be deleted with a proper commit message (it is in the history after all if needed). If it is servers as a reminder of something, a comment should be added saying why it is there instead of leaving the reader wondering.

Commenting out all code would indeed fix all or most of compiler warnings, but that would be certainly an unexpected thing to find behind a "fix warnings commit". 😄

Good point. Although I can live with commented out code (for readily available reminder in case...), it certainly need a commit (with message) on its own. Do you have any suggestion, @metzm?

nilason · 2021-01-30T16:20:52Z

I separated the commented out code in r.stream.extract to a single distinct commit. The commit message may of course be improved...

metzm · 2021-01-31T20:59:17Z

I think, if it is just wrong, it should be deleted with a proper commit message (it is in the history after all if needed). If it is servers as a reminder of something, a comment should be added saying why it is there instead of leaving the reader wondering.
Commenting out all code would indeed fix all or most of compiler warnings, but that would be certainly an unexpected thing to find behind a "fix warnings commit". smile

Good point. Although I can live with commented out code (for readily available reminder in case...), it certainly need a commit (with message) on its own. Do you have any suggestion, @metzm?

Delete the commented out code, no need to keep it.

nilason · 2021-02-01T14:21:28Z

Apparently there is no way for me to merge this without sqashing all commits.
It would be preferable to keep the commits separate. Suggestion anyone?

neteler · 2021-02-02T14:08:55Z

It might work if I cherry pick to master locally and then push. What do you think, shall I give it a try?

Yes, please give it a try.

as Option->answer is declared as as "char *". Addresses -Wc++11-compat-deprecated-writable-strings compiler warnings.

Addresses -Wdangling-else compiler warning.

to respect array bounds: avoid call to array index, which is past the end of the array. (#1248) Addresses -Warray-bounds compiler warnings. Co-authored-by: Markus Metz <[email protected]> Co-authored-by: Huidae Cho <[email protected]>

to respect array bounds: avoid call to array index, which is past the end of the array. (#1248) Addresses -Warray-bounds compiler warnings.

according to given argument type. (#1248) Addresses -Wabsolute-value compiler warnings.

for setting main drainage direction to A* path if possible. (#1248) Co-authored-by: Markus Metz <[email protected]>

nilason · 2021-02-02T14:22:53Z

Merged through cherry picking. Closing this manually.
Thanks a bunch for all the help!

as Option->answer is declared as as "char *". Addresses -Wc++11-compat-deprecated-writable-strings compiler warnings.

Addresses -Wdangling-else compiler warning.

to respect array bounds: avoid call to array index, which is past the end of the array. (OSGeo#1248) Addresses -Warray-bounds compiler warnings. Co-authored-by: Markus Metz <[email protected]> Co-authored-by: Huidae Cho <[email protected]>

to respect array bounds: avoid call to array index, which is past the end of the array. (OSGeo#1248) Addresses -Warray-bounds compiler warnings.

according to given argument type. (OSGeo#1248) Addresses -Wabsolute-value compiler warnings.

for setting main drainage direction to A* path if possible. (OSGeo#1248) Co-authored-by: Markus Metz <[email protected]>

as Option->answer is declared as as "char *". Addresses -Wc++11-compat-deprecated-writable-strings compiler warnings.

Addresses -Wdangling-else compiler warning.

to respect array bounds: avoid call to array index, which is past the end of the array. (OSGeo#1248) Addresses -Warray-bounds compiler warnings. Co-authored-by: Markus Metz <[email protected]> Co-authored-by: Huidae Cho <[email protected]>

to respect array bounds: avoid call to array index, which is past the end of the array. (OSGeo#1248) Addresses -Warray-bounds compiler warnings.

according to given argument type. (OSGeo#1248) Addresses -Wabsolute-value compiler warnings.

for setting main drainage direction to A* path if possible. (OSGeo#1248) Co-authored-by: Markus Metz <[email protected]>

as Option->answer is declared as as "char *". Addresses -Wc++11-compat-deprecated-writable-strings compiler warnings.

Addresses -Wdangling-else compiler warning.

to respect array bounds: avoid call to array index, which is past the end of the array. (OSGeo#1248) Addresses -Warray-bounds compiler warnings. Co-authored-by: Markus Metz <[email protected]> Co-authored-by: Huidae Cho <[email protected]>

to respect array bounds: avoid call to array index, which is past the end of the array. (OSGeo#1248) Addresses -Warray-bounds compiler warnings.

according to given argument type. (OSGeo#1248) Addresses -Wabsolute-value compiler warnings.

for setting main drainage direction to A* path if possible. (OSGeo#1248) Co-authored-by: Markus Metz <[email protected]>

…obust (#6543) This block got removed as part of compiler warnings cleanup #1248, but the removal broke special cases (#6541). The intention of r.stream.extract is to use external flow accumulation for stream extraction, but if in doubt follow the A* search direction, i.e. if (fabs(wat_nbr[np_side]) >= max_acc). Restoring this deleted code block in addition to #6554 should make r.stream.extract robust for special cases as reported in #6541.

nilason mentioned this pull request Jan 13, 2021

[Feat] Fix compiler warnings #1247

Closed

27 tasks

nilason commented Jan 13, 2021

View reviewed changes

raster/r.stream.extract/streams.c Outdated Show resolved Hide resolved

nilason commented Jan 13, 2021

View reviewed changes

lib/raster3d/test/test_put_get_value_large_file.c Outdated Show resolved Hide resolved

nilason commented Jan 13, 2021

View reviewed changes

raster/r.viewshed/main.cpp Outdated Show resolved Hide resolved

nilason added this to the 8.0.0 milestone Jan 13, 2021

nilason added the help wanted Extra attention is needed label Jan 13, 2021

HuidaeCho reviewed Jan 16, 2021

View reviewed changes

imagery/i.aster.toar/main.c Outdated Show resolved Hide resolved

imagery/i.aster.toar/main.c Show resolved Hide resolved

imagery/i.aster.toar/main.c Outdated Show resolved Hide resolved

nilason force-pushed the fix-compiler-warnings-1 branch from b73fe25 to a05c390 Compare January 18, 2021 14:29

nilason commented Jan 18, 2021

View reviewed changes

raster/r.viewshed/main.cpp Outdated Show resolved Hide resolved

nilason force-pushed the fix-compiler-warnings-1 branch 2 times, most recently from 0daa001 to a68cee6 Compare January 18, 2021 16:25

nilason marked this pull request as ready for review January 18, 2021 18:00

nilason requested review from HuidaeCho, metzm and wenzeslaus January 18, 2021 18:09

nilason removed the help wanted Extra attention is needed label Jan 18, 2021

HuidaeCho reviewed Jan 18, 2021

View reviewed changes

imagery/i.aster.toar/main.c Outdated Show resolved Hide resolved

imagery/i.evapo.time/main.c Outdated Show resolved Hide resolved

raster/r.viewshed/main.cpp Show resolved Hide resolved

nilason force-pushed the fix-compiler-warnings-1 branch from 9281283 to a86a4b3 Compare January 22, 2021 10:53

metzm approved these changes Jan 23, 2021

View reviewed changes

wenzeslaus approved these changes Jan 30, 2021

View reviewed changes

nilason force-pushed the fix-compiler-warnings-1 branch from a86a4b3 to d6b1721 Compare January 30, 2021 16:15

nilason force-pushed the fix-compiler-warnings-1 branch from d6b1721 to 8687b82 Compare February 1, 2021 07:40

nilason added a commit that referenced this pull request Feb 2, 2021

Convert literal to mutable string (#1248)

1c358bc

as Option->answer is declared as as "char *". Addresses -Wc++11-compat-deprecated-writable-strings compiler warnings.

nilason added a commit that referenced this pull request Feb 2, 2021

Add explicit braces to avoid dangling "else" (#1248)

7b1c476

Addresses -Wdangling-else compiler warning.

nilason added a commit that referenced this pull request Feb 2, 2021

i.evapo.time: Fix loop indexing

313581c

to respect array bounds: avoid call to array index, which is past the end of the array. (#1248) Addresses -Warray-bounds compiler warnings.

nilason added a commit that referenced this pull request Feb 2, 2021

Use correct absolute value function

6146f05

according to given argument type. (#1248) Addresses -Wabsolute-value compiler warnings.

nilason added a commit that referenced this pull request Feb 2, 2021

r.stream.extract: remove unneeded condition

a36d34e

for setting main drainage direction to A* path if possible. (#1248) Co-authored-by: Markus Metz <[email protected]>

nilason closed this Feb 2, 2021

nilason deleted the fix-compiler-warnings-1 branch February 2, 2021 14:39

marisn pushed a commit to marisn/grass that referenced this pull request Mar 22, 2021

Convert literal to mutable string (OSGeo#1248)

40d696e

as Option->answer is declared as as "char *". Addresses -Wc++11-compat-deprecated-writable-strings compiler warnings.

marisn pushed a commit to marisn/grass that referenced this pull request Mar 22, 2021

Add explicit braces to avoid dangling "else" (OSGeo#1248)

0c2e1c8

Addresses -Wdangling-else compiler warning.

marisn pushed a commit to marisn/grass that referenced this pull request Mar 22, 2021

i.evapo.time: Fix loop indexing

8b01b20

to respect array bounds: avoid call to array index, which is past the end of the array. (OSGeo#1248) Addresses -Warray-bounds compiler warnings.

marisn pushed a commit to marisn/grass that referenced this pull request Mar 22, 2021

Use correct absolute value function

595511a

according to given argument type. (OSGeo#1248) Addresses -Wabsolute-value compiler warnings.

marisn pushed a commit to marisn/grass that referenced this pull request Mar 22, 2021

r.stream.extract: remove unneeded condition

7357733

for setting main drainage direction to A* path if possible. (OSGeo#1248) Co-authored-by: Markus Metz <[email protected]>

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022

Convert literal to mutable string (OSGeo#1248)

95937b6

as Option->answer is declared as as "char *". Addresses -Wc++11-compat-deprecated-writable-strings compiler warnings.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022

Add explicit braces to avoid dangling "else" (OSGeo#1248)

889588a

Addresses -Wdangling-else compiler warning.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022

i.evapo.time: Fix loop indexing

e176b69

to respect array bounds: avoid call to array index, which is past the end of the array. (OSGeo#1248) Addresses -Warray-bounds compiler warnings.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022

Use correct absolute value function

4623ee3

according to given argument type. (OSGeo#1248) Addresses -Wabsolute-value compiler warnings.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022

r.stream.extract: remove unneeded condition

c792c65

for setting main drainage direction to A* path if possible. (OSGeo#1248) Co-authored-by: Markus Metz <[email protected]>

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023

Convert literal to mutable string (OSGeo#1248)

51aeda1

as Option->answer is declared as as "char *". Addresses -Wc++11-compat-deprecated-writable-strings compiler warnings.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023

Add explicit braces to avoid dangling "else" (OSGeo#1248)

336b17d

Addresses -Wdangling-else compiler warning.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023

i.evapo.time: Fix loop indexing

4ed9edd

to respect array bounds: avoid call to array index, which is past the end of the array. (OSGeo#1248) Addresses -Warray-bounds compiler warnings.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023

Use correct absolute value function

afd3675

according to given argument type. (OSGeo#1248) Addresses -Wabsolute-value compiler warnings.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023

r.stream.extract: remove unneeded condition

63255e5

for setting main drainage direction to A* path if possible. (OSGeo#1248) Co-authored-by: Markus Metz <[email protected]>

petrasovaa mentioned this pull request Oct 24, 2025

r.stream.extract: fix stream delineation for binary flow accumulation #6543

Merged

Uh oh!

Comments

Conversation

nilason commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nilason left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nilason commented Jan 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metzm commented Jan 21, 2021

Uh oh!

nilason commented Jan 22, 2021

Uh oh!

metzm left a comment

Choose a reason for hiding this comment

Uh oh!

nilason commented Jan 29, 2021

Uh oh!

wenzeslaus left a comment

Choose a reason for hiding this comment

Uh oh!

nilason commented Jan 30, 2021

Uh oh!

nilason commented Jan 30, 2021

Uh oh!

metzm commented Jan 31, 2021

Uh oh!

nilason commented Feb 1, 2021

Uh oh!

neteler commented Feb 2, 2021

Uh oh!

nilason commented Feb 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nilason commented Jan 13, 2021 •

edited

Loading