Remove redunant definitions

[jarkkojs]

No description provided.

[jarkkojs]

Closes three issues.

[esaruoho]

@kurasu @baconpaul should this be merged in?

[esaruoho]

from my end, i'm not seeing which three issues it closes, since they are not referenced by number (hash-followed-by-number, like #1 ), and the original PR has no description nm

[baconpaul]

The commit msg has the numbers

I am not sure the sse version is metgable and still am without a laptop so left it until I can look

I know it’s there though. No need from my side to put pings on prs or issues

[esaruoho]

from commit text
Fixes: #172 ("Remove SSE_VERSION macro")
Fixes: #173 ("Remove stricmp=strcasecmp definition from premake5.lua")
Fixes: #174 ("Remove __cdecl definition in the Linux target in premake5.lua?")
Signed-off-by: Jarkko Sakkinen [email protected]

[baconpaul]

OK the only reason I am not merging this is because of the vst2/vst2plugsquartz.h removal of the #define. I haven't been able to get a windows image to see if that defined variable is used inside any of the subsequent headers. (I can confirm it is not on linux and mac which is why I'm fine with all the other changes).

Have you either had a build with sse and vst2 work properly on windows with that change, have access to the headers for a grep -r, or mind backing that one diff out?

Or maybe @kurasu is more comfy knowing the purpose of that variable on windows and then can merge it. I'm just not sure enough to press merge above.

[kzantow]

stupid question: is there a standard for this function?

[baconpaul]

There isn't! Annoying right? If you squint you can convince yourself that c strings are memory blocks so ideas like 'case' are encoding specific, then get into wchar and encodings (and maybe even ebcdic!). So everyone needs to define one as the other or vice versa somewhere (which @whydoubt noted over on #173).

I'm not quite sure where that re-definition has gone in this PR, either. @jsakkine ?

[baconpaul]

And as I noticed over on the issue I read this diffs backwards. We should not add a platform specific ifdef every time we do a stricmp. I added 3 more to squash a bug yesterday for instance

So if we are taking out of the premake it’s gotta go in a header somewhere and then we can just code to stricmp everywhere. I don’t think we should merge this change

[jarkkojs]

@baconpaul, I can drop the vst2plugsquartz.h change, sure. Should I do it?

[baconpaul]

Yeah just to be conservative

We need to change the stricmp one also I think. Where do we want to define stricmp=strcasecmp? The platform if around every invocation is a bit too sloppy.

[jarkkojs]

Sure, NP, I'll get over my paid work and work on Surge issues tonight :) Finally through the holiday season...

[jarkkojs]

I updated this and work on GUI tonight when I get back from work...

[baconpaul]

Cool. I think the general mood is the stricmp should be defined once someplace also so we don't need those ifs. So remove it from the premake but I think in src/common/globals.h add an

#if mac || linux
#define stricmp stcasecmp
#endif 

then back out the other #if macs and we can just use stricmp everywhere with no premake required.

I also noticed global.h does a global using namespace std. Sigh. One day!

[jarkkojs]

@baconpaul, git grep of the use of stricmp() or strcasecmp() shows that there are total three call sites:

src/common/Sample.cpp:   if (!stricmp(extension, "wav"))
src/common/dsp/DspUtilities.h:   if (_stricmp(str, "yes") == 0)
src/common/gui/CSurgeSlider.cpp:   if (!stricmp(txt, "filter balance"))

One of the call sites in DspUtilities.h looks like this:

inline bool is_yes(const char* str)
{
   if (!str)
      return 0;
#if WINDOWS
   if (_stricmp(str, "yes") == 0)
      return 1;
#else
   if (strcasecmp(str, "yes") == 0)
      return 1;
#endif
   return 0;
}

The current patch actually makes thing more consistent than they were before. The most clean way IMHO to sort this out would be just to define an inline function and use it in all three call sites instead of macro substitutions. Works better with IDEs, can be namespaced etc. and is as lightweight.

BTW, Microsoft has deprecated stricmp so that inline function should actually use _stricmp: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/stricmp-wcsicmp?view=vs-2017

[baconpaul]

@jsakkine you should pull latest master. You'll see 6 then.

I think yes we should introduce an inlined function in global.h and use that consistently. I'm fine with whatever we want to call it. I just don't want to always have to guard a case insensitive string comparison.

So add if mac | linux int _stricmp (a b ) { return strcasecmp( a, b ); } would be fine with me if you prefer.

[baconpaul]

oh and when you do the menu sorting patch, my bet is you will add two more! Chuckle.

[jarkkojs]

Updated.

[jarkkojs]

I'll hold up until this is merged so I have kind of clean slate and work on menus when I wake up based on your good suggestions :) Thank you for patience. I can be bugging sometimes with my opinions...

[baconpaul]

Great. That builds and works. I'll merge it now. Thanks also for your patience!