commontk / AppLauncher

Simple and small program allowing to set the environment of any executable.
http://www.commontk.org
Apache License 2.0
31 stars 32 forks source link

BUG: fix crash calculating environment keys #117

Closed pieper closed 4 years ago

pieper commented 4 years ago

Fixes #116

Each call to ctkAppLauncherEnvironment::excludeReservedVariableNames returns a new QStringList so making a QSet from the begin() of one and the end() of the other led to the crash in the QHash.

Solution is to make temporary variables for the QStringLists so that the set is made from matching iterators variables.

Also rearranged the code a bit for readability.

pieper commented 4 years ago

I've tested this and it works on my local mac build.

lassoan commented 4 years ago

Awesome! Thanks a lot for these investigations and fixes.

pieper commented 4 years ago

Would this syntax work, too?

QSet<QString> currentSet(currentStrings.begin(), currentStrings.end());

No, it's a constructor so it needs the parens or you get this:

/opt/s/CTKAppLauncherLib/Base/ctkAppLauncherEnvironment.cpp:108:35: error: expected '(' for function-style cast or type construction
  auto currentSet = QSet<QString> currentStrings.begin(), currentStrings.end();
                    ~~~~~~~~~~~~~ ^
pieper commented 4 years ago

Would this syntax work, too?

QSet<QString> currentSet(currentStrings.begin(), currentStrings.end());

No, it's a constructor so it needs the parens or you get this:

/opt/s/CTKAppLauncherLib/Base/ctkAppLauncherEnvironment.cpp:108:35: error: expected '(' for function-style cast or type construction
  auto currentSet = QSet<QString> currentStrings.begin(), currentStrings.end();
                    ~~~~~~~~~~~~~ ^

Oh, I see what you mean - yes, that's cleaner. Thanks!

jcfr commented 4 years ago

Thanks for the fix :pray: