Warnings and analyzer issues

[jobi-wan]

This branch addresses a bunch of compiler warnings and analyzer issues.
See individual commits for details.

[AnotherCommander]

OK, this is a lot of stuff to check. Generally it looks OK and most of it can be committed, but I have a few comments.

Fitst of all, now that we have entered feature freeze, the objective of every commit is to either improve documentation or fix broken behaviour. This PR contains various commits for fixing compiler warnings and, in principle, is OK to review for merge. However, if there are any doubts about the functionality of a specidic patch, I would prefer to keep the compiler warning than having to fix something that does not affect anything in the game code. With that in mind, the commits that I would like to discuss are:

• 9fe960b: The issue here is that the legal status is not exactly an OOCreditsQuantity variable. As the comment in PlayerEntity.h very eloquently puts it: "legalStatus both is and isn't an OOCreditsQuantity, because of quantum." So I am not sure that declaring the offset_value in -markAsOffender: as OOCreditsQuantity is the right way to go. What I would do here rather than changing the -markAsOffender: return value,, given that we only have two occurences of the warning, would be to change the two offending lines to"

[self markAsOffender:(int)[dockedStation legalStatusOfManifest:shipCommodityData export:NO] withReason:kOOLegalStatusReasonIllegalImports];

[self markAsOffender:(int)[station legalStatusOfManifest:shipCommodityData export:YES] withReason:kOOLegalStatusReasonIllegalExports];

• eb8ad57: I would suggest using the more standard int32_t instead of int32.
• 0122ace: I cannot test this one in-game because I have no joystick. I would be OK to merge this, if and only if we are absolutely sure that the int type can be substitued by the NSUInteger one safely. That is, if we are sure that there are no checks anywhere for any of those changed type variables for a return of -1, for example. I know that in the SDL code we do use -1 often for checking returns, so if any of those variables is used in such a way, then we could end up trying to fix a warning and breaking the joystick in SDL. In case of doubt, I would rather leave those warnings alone and merge this part post-1.86.

The rest seems OK to me and I see no problem merging it.

[jobi-wan]

I realized we entered code freeze, and we could just leave all of these until after release of 1.86. They don't actually address any real problems anyway, just make compilation cleaner. (On for xcode 64 bit at least. Might even introduce new warnings for other platforms?)

I didn't mean to suggest to merge all of this wholesale. Especially not at this time. Would it be better for things like this to add comments in the code? (On github, by clicking the blue + , not in the actual code..) An 'attention request' rather than a 'pull request' so to speak..

• 9fe960b: Will look into that again.
• eb8ad57: I would agree, but the prototype of the called function is INT_TO_JSVAL(int32 i)
• 0122ace: I looked at it and it seems these were used for indices and counts. I can't test it either.

Question:

• ab59179: For example. This if has a negative condition and (now) an else-block. Personally, I don't like that. I'd rather make the condition positive (change the != into == ) and swap the 'then' and 'else' blocks.
  On the other hand, that would make the difference unnecessarily bigger, possibly making it more difficult down the line to compare across larger time frames.
  Is there a policy or consensus on things like this to favor one over the other?

[AnotherCommander]

Right now the problem I am faced with is that I am not sure how to commit the parts that seem no problem and exclude the three commits that are under discussion. Compiler warning fixes are fine for code freeze, provided they do not change the behaviour of the game so the majority of the PR could go in, I am just not sure how. I think that at least the analyzer fixes to potential null pointer dereferences are particularly important and should go in.

@jobi-wan since you are now a member of the project with write access, maybe it would be easier if you pushed the changes that are good to go directly to the repository. I can do it myself, if you prefer, just let me know.

Regarding the commits:

• eb8ad57: You are right about the prototype, but I would still prefer the int32_t, because it is the one that is guaranteed to exist on any C compliant compiler and, most importantly, is what we have used elsewhere in the code when we cast a 32-bit integer for INT_TO_JSVAL.
• 0122ace: If we cannot test it, I'd rather leave it for later.

Regarding the if with a negative condition, I don't think there is any particular consensus or guideline about how to handle such situations. What I normally do is just keep the style used by the original coder when editing someone else's code and try to follow a logic aligned to what you propose when I write new code.

[jobi-wan]

As per your email, commits 694227f, ab59179, and f80eeb3 from this PR have been pushed as aac51d6, c24bb8d, and e0f311b respectively and merged into master in commit 0819f1b.
(They got new IDs because I had to git format-patch them from my fork and then git am then into this repo.)

[jobi-wan]

Shall I push da2f747 and 2bd99d0 ?

[AnotherCommander]

Yes, please go ahead.

[jobi-wan]

Done.
da2f747 and 2bd99d0 from this branch in my fork are now 54b6206 and 2855cc9 in this repo.
Merged in 9a1b4a2