dMZX Forums: String Equalities are Case Insensitive: If Intended, Not Documented. -> Archived Requests -> Tracker

18:57 < Insidious> hmm
18:57 < Insidious> there seems to be a deficiency in MZX's string comparisons.
18:58 < Insidious> if "$string" = "J" "shiggityblam"
18:58 < Insidious> that will trigger if string is J or j
18:58 < Insidious> string comparisons should not imo be case insensitive, or at
least, there should be a case sensitivity option
19:00 < nooodl> what, really
19:00 < Insidious> yeah, I just came across that problem because I had J
signifying Jack and j signifiying Jane
19:00 < Insidious> and it kept thinking Jack had two turns and jane had none
19:01 < nooodl> hmm. you're right
19:01 < Insidious> (because the comparison between the letter and J came first
and blocked the comparison between it and j)
19:01 < Insidious> jane is now Q.
19:02 < nooodl> is this even documented anywhere
19:02 < Insidious> I doubt it.
19:02 < nooodl> the entry on IF string equality string doesn't mention it
19:02 < nooodl> file a bug


Basically, this example

set "$string" "j"
if "$string" = "J" "klarf"
* String does not equal uppercase J
end
:klarf
* String equals uppercase J
end

will output "String equals uppercase J" even when this is not true.

I ran into this issue when using a string as a surrogate array, and pulling elements out to determine turn order in my RPG. J stood for the 1st character, codenamed "Jack" and j stood for the second, codenamed "Jane". This bug, due to the structure of the code, caused Jack to show up in the turn order twice instead of Jack and then Jane.

Here's the function that handles string comparisons everywhere[1] in MZX:

int compare_strings(struct string *dest, struct string *src)
{
  char *src_value;
  char *dest_value;
  Uint32 *src_value_32b;
  Uint32 *dest_value_32b;
  Uint32 val_src_32b, val_dest_32b;
  Uint32 difference;
  char val_src, val_dest;
  size_t cmp_length = dest->length;
  size_t length_32b, i;

  if(src->length < cmp_length)
    return 1;

  if(src->length > cmp_length)
    return -1;

  // Compare 32bits at a time, should be mostly portable now
  length_32b = cmp_length / 4;

  src_value = src->value;
  dest_value = dest->value;

  src_value_32b = (Uint32 *)src_value;
  dest_value_32b = (Uint32 *)dest_value;

  for(i = 0; i < length_32b; i++)
  {
    val_src_32b = src_value_32b[i];
    val_dest_32b = dest_value_32b[i];

    if(val_src_32b != val_dest_32b)
    {
      difference =
       toupper(val_dest_32b >> 24) - toupper(val_src_32b >> 24);

      if(difference)
        return difference;

      difference = toupper((val_dest_32b >> 16) & 0xFF) -
       toupper((val_src_32b >> 16) & 0xFF);

      if(difference)
        return difference;

      difference = toupper((val_dest_32b >> 8) & 0xFF) -
       toupper((val_src_32b >> 8) & 0xFF);

      if(difference)
        return difference;

      difference = toupper(val_dest_32b & 0xFF) -
       toupper(val_src_32b & 0xFF);

      if(difference)
        return difference;
    }
  }

  for(i = length_32b * 4; i < cmp_length; i++)
  {
    val_src = src_value[i];
    val_dest = dest_value[i];

    if(val_src != val_dest)
    {
      difference = toupper((int)val_dest) - toupper((int)val_src);

      if(difference)
        return difference;
    }
  }

  return 0;
}

As you can see, it's case insensitive (as it touppers everything). Changing this would break games, and would have to be version checked. I'd prefer it if the documentation was changed to reflect this (mis)feature.

EDIT: [1] IF is really the only place this is obviously done

--ajs.

Yeah, my big problem is that it wasn't documented, and thus I didn't know about it before I tried it and it didn't work. I totally understand not fixing it as of yet.

EDIT: #mzx is bringing up the possibility of instead of changing if "$string" = "blah", having a case sensitive comparator, maybe as === or so. Apparently there is room in the comparison operators for this according to lolilover?

New comparator time. How about === for case sensitive string comparing?

Okay. It's a moderately complex change since we need to update legacy_rasm with the new operator and check for bugs in the assembler/disassembler. Then plumb it through to run_robot. But it's pretty self contained because this is only used by one command.

It presents an incompatibility that downver can't handle, which is new, but we can probably modify it to special-case the translation of === to plain = to at least allow worlds to load in older versions.

I'm not going to implement this right away. I want to make sure I've considered all of the compatibility issues.

--ajs.

Updating status to: Flagged For Future Version

Updating version to: None

I don't think it'd hurt too much to put this off until debytecode, if you REALLY need case-sensitive checking, which is rare in MZX, you could easily write it manually. No legacy_rasm issues to deal with at that point.

set "$str1" "BEEF"
set "$str2" "BEEFz"
goto "#compDisp"

set "$str1" "Hello world"
set "$str2" "Hello World"
goto "#compDisp"

set "$str2" "Hello world"
goto "#compDisp"
end

: "#comp"
set "LOCAL0" "LOOPCOUNT"
set "LOCAL" "('$str1.length&'-1)"
set "LOOPCOUNT" -1
if "$str1.length" != "$str2.length" "skip"
set "LOOPCOUNT" 0
if "LOCAL" = -1 "skip"
loop start
loop "('$str1.&LOOPCOUNT&'='$str2.&LOOPCOUNT&'*'LOCAL')"
: "skip"
set "same" "('LOOPCOUNT'='$str1.length')"
set "LOOPCOUNT" "LOCAL0"

. "display"
* "~f&same&"
wait 30
clear mesg
wait 5
goto "#RETURN"

Moving to: MegaZeux Feature Requests

I'm getting sick of seeing this in bug reports. This is obviously the intended functionality (look at the rest of the language!!!!!!!!!!!), and there will be a new operator added for case sensitive compares.

Updating version to: 2.85

Updating version to: GIT (debytecode)

Updating status to: Approved
Updating version to: 2.91

This can be done in 2.91 since 2.90d won't explode if this goes through downver. This needs to be added to both assemblers.

Updating status to: Implemented
Issue fixed in: 2.91

Implemented for 2.91 and debytecode in GIT 1454ff5 with fixes for debytecode in GIT aa4cc2d and GIT f9773b2.

Moving to: Archived Requests