Project

General

Profile

Bug #2415

Subtle bug in PartModuleList this[string className]

Added by swamp_ig about 10 years ago. Updated over 7 years ago.

Status:
Closed
Severity:
Normal
Assignee:
-
Category:
Application
Target version:
-
Start date:
04/23/2014
% Done:

100%

Version:
Platform:
Any
Expansion:
Language:
English (US)
Mod Related:
No
Votes:
Arrow u r green
Arrow d r red

Description

    public PartModule this[string className]
    {
        get
        {
            // Change this variable to whatever your private variable is obfiscated from
            List<PartModule> theList = null; 
            int hashCode = className.GetHashCode();
            for (int i = 0; i < theList.Count; i++)
            {
                PartModule partModule = theList[i];
                // DANGER! never compare hash codes without also comparing equality.
                // you can get strings that hash to the same value. 
                // Besides, native string comparison does this already so there's really no performance gain
                if (partModule.ClassName == className)
                    return partModule;
            }

            // This isn't your error, but you get the idea.
            Debug.LogError("Unable to get class " + className + " from PartModuleList");
            return null
        }
    }

History

#1 Updated by swamp_ig about 10 years ago

Note: the 7th line is superfluous obviously.

#2 Updated by swamp_ig about 10 years ago

What might also be a good option would be to change PartModuleList so it extends System.Collections.ObjectModel.KeyedCollection<string, PartModuleList>

That way you're extending an API class which you know works.

There's a few places where you are using a string hash-code in the place of an actual string. This really isn't a good idea and will result in subtle, hard to detect bugs.

It's probably actually going to be slower than native string comparison too. The compiler collects all strings that are defined statically into the same object, and you can call string.Intern on any strings defined dynamically to internalize them in the same way. That way comparison is done on object identity, so it's very quick.

#3 Updated by TriggerAu almost 8 years ago

  • Status changed from New to Needs Clarification

#4 Updated by TriggerAu over 7 years ago

  • Status changed from Needs Clarification to Closed
  • % Done changed from 0 to 100

Closing this report out for now. If you find it is still occuring in the latest version of KSP please open a new report (and this one can be linked to it.) For best results, the wiki contains really useful info for when creating a report http://bugs.kerbalspaceprogram.com/projects/ksp/wiki.

You can also ask questions about the bug cleanup in the forum here: http://forum.kerbalspaceprogram.com/index.php?/topic/143980-time-to-clean-up-the-bug-tracker/ and tag @TriggerAu to get my attention

Also available in: Atom PDF