• GeneralDiscussion
• Negative skill potential quirks

[I debated whether to put this under BugReports, but I don't think it quite qualifies. Feel free to relocate this if I've erred. -- ZizzoTheInfinite]

ZizzoTheInfinite: I've been examining the skills code pursuant to updating the skill spoiler for 2.3.x, and I've come across a potential ambiguity which might cause negative skills to be handled differently on different computers. The basic get_skill() function remains unchanged:

s16b get_skill(int skill)
{
        return (s_info[skill].value / SKILL_STEP);
}

except that s_info[skill].value can now be negative. This can lead to a problem described in section 6.3.5 of the ANSI/ISO C standard, which says:

• [...] If either [integer] operand is negative, whether the result of the / operator is the largest integer less than or equal to the algebraic quotient or the smallest integer greater than or equal to the algebraic quotient is implementation-defined, as is the sign of the result of the % operator. If the quotient a/b is representable, the expression (a/b)*b + a%b shall equal a.

Translated to English, this means that if s_info[i].value is -2900, then it's implementation-defined (ie. depends on the computer and/or the compiler) whether get_skill(i) will return -2 or -3—and, for the same reason, whether eg. get_skill_scale(i, 100) will return -5 or -6. If the intent is to round toward zero on both sides, we should probably disambiguate it with something like this:

s16b get_skill(int skill)
{
        s16b temp = abs(s_info[skill].value) / SKILL_STEP;
        return temp * (s_info[skill].value < 0 ? -1 : 1);
}

and similarly for get_skill_scale().

This is also related to the funky display of negative skills, and there's a potential for skill values to be displayed incorrectly. The key is the relationship between / and % described in the standard quote above. dump_skills() has the following code:

       fprintf(fff, "%-50s%02ld.%03ld [%01d.%03d]",
               buf, s_info[i].value / SKILL_STEP, s_info[i].value % SKILL_STEP,
               s_info[i].mod / 1000, s_info[i].mod % 1000);

and print_skills() has similar code. Now, suppose again that s_info[i].value is -2900. If the compiler evaluates -2900/1000 as -2, then it's required to evaluate -2900%1000 as -900, leading to the now-familiar odd formatting of "-02.-900". If the compiler evaluates -2900/1000 as -3, however, it's required to evaluate -2900%1000 as 100, causing the code to print a skill value of "-03.100", which is better formatted but wrong. Again, we can fix this by pulling out the leading minus separately and using abs() everywhere else, something like this:

       fprintf(fff, "%-49s%c%02ld.%03ld [%01d.%03d]",
               buf, (s_info[i].value < 0 ? '-' : ' '),
        abs(s_info[i].value) / SKILL_STEP, abs(s_info[i].value) % SKILL_STEP,
               s_info[i].mod / 1000, s_info[i].mod % 1000);

which should properly format as "-02.900" in both cases. The code in print_skills() can be fixed similarly. I'm assuming here that s_info[i].mod can't be negative; if it can, similar corrective measures will be required.

NeilStevens: Good finds. Since DG already took care of my next thing to do in ToME, this will probably be my new next thing to do.

ZizzoTheInfinite: Another thing on this front that will need to be looked at is this line from src/xtra1.c:

        p_ptr->skill_sav += adj_wis_sav[get_skill_scale(SKILL_SPIRITUALITY, 37)];

If the Spirituality skill is negative, this may try to access data outside the adj_wis_sav[] array, with unpredictable results. The lowest value in the array is zero, so I suppose we could use that for negative skill values—or apply a negative modification in that case if we really wanted to be mean…

GreyCat: Neil, I've already fixed this in tome_230_branch. Thanks to Zizzo for pointing out the subtle trap on various platforms.