Project-New-Reign---Nemesis-Main icon indicating copy to clipboard operation
Project-New-Reign---Nemesis-Main copied to clipboard

Nearly everything in 'src/utilities/algorithm.cpp' is fundamentally flawed and guaranteed to cause memory leaks 100% of the time

Open Akira13641 opened this issue 2 years ago • 5 comments

Here is a Compiler Explorer link that uses a standalone version of algorithm.cpp to demonstrate the overall "domino effect" problem by calling your version of iequals a couple of times.

Akira13641 avatar Dec 15 '21 19:12 Akira13641

No wonder this app crashes so much when tou try to generate behavior with large amounts of animations

BullOnMars avatar Dec 16 '21 12:12 BullOnMars

That would be the cause yes. Memory leaks are no joke

DMNerd avatar Dec 16 '21 12:12 DMNerd

No wonder this app crashes so much when tou try to generate behavior with large amounts of animations

Yeah, the implications of stuff like the use of their version of iequals (which results in two always-leaked allocations per call) in places like this nested loop are uh, far from great to say the least.

Akira13641 avatar Dec 16 '21 15:12 Akira13641

If not linking Boost, the answer in this question may be useful as an easy replacement for case insensitive string equals. It neatly avoids doing any needless data duplication that could result in leaks. I'm not familiar with this "wstring" type though, so might need a duplicate or conversion for that type.

Tyler799 avatar Dec 17 '21 21:12 Tyler799

Definitely some good solutions provided there, yeah. I'd personally probably use classic glibc-style functions as a baseline though (with the insensitive comparison built in), as they optimize very well under most compilers, and would allow for all the same overloads of iequals to be built on them. Something like this for example (possibly with some kind of preventative check for null pointers added to the first two):

int istrcmp(const char* left, const char* right) {
  const unsigned char* s1 = (const unsigned char*)left;
  const unsigned char* s2 = (const unsigned char*)right;
  unsigned char c1, c2;
  
  do {
    c1 = (unsigned char)tolower(*s1++);
    c2 = (unsigned char)tolower(*s2++);
    if (c1 == '\0')
      return c1 - c2;
  } while (c1 == c2);
  
  return c1 - c2;
}

int iwcscmp(const wchar_t* left, const wchar_t* right) {
  wchar_t c1, c2;

  do {
    c1 = tolower(*left++);
    c2 = tolower(*right++);
    if (c2 == L'\0')
      return c1 - c2;
  } while (c1 == c2);

  return c1 < c2 ? -1 : 1;
}

bool iequals(const char* l, const char* r) { return istrcmp(l, r) == 0; }

bool iequals(const wchar_t* l, const wchar_t* r) { return iwcscmp(l, r) == 0; }

bool iequals(const char* l, const string& r) { return istrcmp(l, r.c_str()) == 0; }

bool iequals(const wchar_t* l, const wstring& r) { return iwcscmp(l, r.c_str()) == 0; }

bool iequals(const string& l, const char* r) { return istrcmp(l.c_str(), r) == 0; }

bool iequals(const wstring& l, const wchar_t* r) { return iwcscmp(l.c_str(), r) == 0; }

bool iequals(const string& l, const string& r) { return istrcmp(l.c_str(), r.c_str()) == 0; }

bool iequals(const wstring& l, const wstring& r) { return iwcscmp(l.c_str(), r.c_str()) == 0; }

I think MSVC specifically at least does itself provide non-standard case-insensitive versions of strcmp and wcscmp also, so those would be worth looking at too.

Akira13641 avatar Dec 17 '21 22:12 Akira13641