Conversation
|
All the appveyor errors were for spdlog. I don't think this commit touched anything in libs at all, let alone spdlog so I'm not sure what's going on. Everything built for me fine locally, tests passed, the game ran.
One difference I have locally is that my spdlog.vcxproj has PlatformToolset set to v142 instead of v140, since I don't have the 140 toolchain installed (I'm using VS2019) |
MHeasell
left a comment
MHeasell
left a comment
There was a problem hiding this comment.
Got about half way through here. Vast majority of the changes looked good but there are a couple issues so far that would make me hesistant to merge, in the cob area.
src/rwe/GameNetworkService.cpp
Outdated
|
|
||
| // if the packet is relevant (contains new information), process it | ||
| if (firstRelevantCommandIndex < static_cast<unsigned int>(message.command_set_size())) | ||
| if (firstRelevantCommandIndex < static_cast<int>(message.command_set_size())) |
There was a problem hiding this comment.
message.command_set_size() is already int, could remove this cast altogether?
src/rwe/cob/CobExecutionContext.cpp
Outdated
| } | ||
|
|
||
| unsigned int CobExecutionContext::nextInstruction() | ||
| int CobExecutionContext::nextInstruction() |
There was a problem hiding this comment.
instruction opcodes are also probably better left as unsigned? Since they go into a big switch to compare against the opcode literals in OpCode which do make use of the high bit
There was a problem hiding this comment.
yeah in retrospect I should have left more of the cob stuff alone - I'll change these
There was a problem hiding this comment.
in fact a few of the headers specify uint32_t so I'll use that explicitly for the returns where it makes sense. Easier to read, and usually when I see those I think of bit masks etc which I think is the right thing here (let me know if I'm wrong)
src/rwe/grid/DiscreteRect.test.cpp
Outdated
| } | ||
|
|
||
| rc::prop("a unit rectangle is adjacent if the point is", [](int x, int y, unsigned int w, unsigned int h, int x2, int y2) { | ||
| rc::prop("a unit rectangle is adjacent if the point is", [](int x, int y, int w, int h, int x2, int y2) { |
There was a problem hiding this comment.
this will now generate invalid DiscreteRect instances
There was a problem hiding this comment.
This I still need to fix... have to sit down and understand the test better. Reading it quickly I'm confused what's going on
There was a problem hiding this comment.
Ah, forgot it's x, y, width, height.
It's a bit strange to me that this test tests the unsigned int rollover behavior for the edges, where intuitively I would think that should never happen in real usage. The game seems to play fine using ints and the asserts don't trigger, so I'll change the test data to only use positive ints.
|
|
||
| DiscreteRect() = default; | ||
| DiscreteRect(int x, int y, unsigned int width, unsigned int height) : x(x), y(y), width(width), height(height) | ||
| DiscreteRect(int x, int y, int width, int height) : x(x), y(y), width(width), height(height) |
There was a problem hiding this comment.
I wonder if the constructor should assert or throw if width and height are <0 now that it is possible.
There was a problem hiding this comment.
Just brainstorming... So if it is possible they are <0 now, it was possible before that arithmetic errors(?) before would've produced enormous unsigned width/height values. Those also would be bugs, but uncaught. So I guess it could be argued that yes, we should put in an assert, and that is an improvement vs the previous unsigned code. Thoughts @MHeasell ?
| int x; | ||
| int y; | ||
| GridCoordinates() = default; | ||
| GridCoordinates(size_t x, size_t y) : x(x), y(y) | ||
| GridCoordinates(int x, int y) : x(x), y(y) | ||
| { | ||
| } |
There was a problem hiding this comment.
I wonder what to do here since GridCoordinates can now hold negative values (and can be default-constructed with them) which is invalid for this struct.
There was a problem hiding this comment.
I'm thinking we can put an assert in the constructor, same reasoning as for DiscreteRect.h
There was a problem hiding this comment.
Thinking more, since GridCoordinates and DiscreteRect need the default constructor (as they're used in some other structs that way), the asserts in the non-default constructor don't cover them entirely. I still think the issue is not changed vs unsigned, since the same mistake/error case exists with unsigned (manifesting as enormous numbers with the highest bit set)
There was a problem hiding this comment.
Applies to GridRegion and Grid too
KevinHake
force-pushed
the
remove_unsigned_int
branch
from
34b604e to
f495f8e
Compare
| rc::prop("a unit rectangle is adjacent if the point is", [](int x, int y, int w, int h, int x2, int y2) { | ||
| rc::prop("a unit rectangle is adjacent if the point is", []() | ||
| { | ||
| auto x = *rc::gen::positive<int>(); |
There was a problem hiding this comment.
@MHeasell let me know if this is the right/ideal way to do this
2 participants
Tried to keep this one only unsigned to int changes. Touches a ton of files. I left libs alone, along with a few things like network_util.