Skip to content

Commit

Permalink
add asserts to ter_map::get
Browse files Browse the repository at this point in the history 
this was previously omitted for performance but it helps debugging #2571
  • Loading branch information
@gfgtdf
gfgtdf committed Mar 1, 2018
1 parent 9489875 commit 8dc1613
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/terrain/translation.hpp
View file
Expand Up @@ -89,8 +89,8 @@ namespace t_translation {
ter_map & operator= (ter_map &&) = default;
#endif

terrain_code& get(int x, int y) { return data[x * h + y]; }
const terrain_code& get(int x, int y) const { return data[x * h + y]; }
terrain_code& get(int x, int y) { size_t index = x * h + y; assert(index < data.size()); return data[index]; }
const terrain_code& get(int x, int y) const { size_t index = x * h + y; assert(index < data.size()); return data[index]; }

std::vector<terrain_code> data;
int w;
Expand Down

9 comments on commit 8dc1613

@Vultraz
Copy link
Member

@Vultraz Vultraz commented on 8dc1613 Mar 1, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer if you used std::map::at.

@gfgtdf
Copy link
Contributor Author

@gfgtdf gfgtdf commented on 8dc1613 Mar 2, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at throws an exception, this is an assert which is something different. Most importantly it shows different intentions. But it also easier to debug sich it since it will fail immidateley with line number and possibly a stacktrace instead of just printing a 'caught a out of range exception'

@Vultraz
Copy link
Member

@Vultraz Vultraz commented on 8dc1613 Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know. I just think it's cleaner to use at here, especially since you can add relevant error messages.

@Vultraz
Copy link
Member

@Vultraz Vultraz commented on 8dc1613 Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does't really matter tho.

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gfgtdf I fail to see the advantage of an assert over an exception. Both cause the program to terminate with an error message, after all; and the exception can be caught if necessary, while the assert can't.

I guess it's true that an exception won't include a line number in the message (but really, having a line number won't help you that much here anyway), but you can get a stack trace from an exception about as easily as you can get one from an assert (assuming you have a debugger attached, you basically don't need to do anything). The only downside is that Wesnoth currently catches all exceptions on exit, obscuring this information (perhaps it shouldn't do this?).

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vultraz What do you mean you can add relevant error messages with at? Pretty sure there's no way to do that?

@gfgtdf
Copy link
Contributor Author

@gfgtdf gfgtdf commented on 8dc1613 Mar 2, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gfgtdf I fail to see the advantage of an assert over an exception. Both cause the program to terminate with an error message, after all

This is not guaranteed for a throw, a random catch somewhere in the stacktrace might prevent it from doing that, in particular, since out_of_range is a quite generic exception that is thown by many functions. One code might for exampel have stuff like map_[sides->at(index).leader().location()] in a catch ( out_of_range) intending to catch for the case that index it out of range ...

and the exception can be caught if necessary, while the assert can't.

This is exactly why we are not using it here, we want to add debug information so we add debug information, if we at some point are in some situation where it makes sense to change the interface of that function to 'allow' out of range values we can still add such a check later.

EDIT: in fact we do such things, see https://github.com/wesnoth/wesnoth/blob/1.13.11/src/pathfind/pathfind.cpp#L544

@Vultraz
Copy link
Member

@Vultraz Vultraz commented on 8dc1613 Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CelticMinstrel I was thinking along the lines of:

try {
    return data.at(x * h + y);
catch(const std::out_of_range&) {
    ERR_SOMETHING << "Attempted to access invalid hex location in terrain data cache\n";

    static terrain_code blank_code;
    return blank_code;
}

@AI0867
Copy link
Member

@AI0867 AI0867 commented on 8dc1613 Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to have a fallback if WML does something stupid. If there's a logic error in the code though, we want it to fail as loudly as possible, so it will actually get fixed.

Please sign in to comment.