Tracktion / choc

A collection of header only classes, permissively licensed, to provide basic useful tasks with the bare-minimum of dependencies.
Other
543 stars 49 forks source link

Gift that keeps giving #63

Closed Anthony-Nicholls closed 2 weeks ago

Anthony-Nicholls commented 2 weeks ago

Unfortunately the recent change to avoid GetExceptionCode() == STATUS_STACK_OVERFLOW caused another warning (warning C6320: Exception-filter expression is the constant EXCEPTION_EXECUTE_HANDLER. This might mask exceptions that were not intended to be handled.) however, as you had already discovered the use of GetExceptionCode() and STATUS_STACK_OVERFLOW were also causing issues because Windows.h wasn't included. In this PR I think I've managed to fix all those issues 🤞 and avoid any pragma ignore warning malarky too. Hopefully that will be the last of it!

julianstorer commented 2 weeks ago

Oh god..

Just stepping back for a moment here, this is all just so we can throw an exception if there's a stack overflow... Is it actually possible to throw a C++ exception when there's a stack overflow? Or is there another version of alloca on windows that returns an error value instead of throwing?

julianstorer commented 2 weeks ago

..I guess what's bothering me is the idea that choc_Value.h needs to include windows.h just to handle this ridiculously edge-casey situation..

Anthony-Nicholls commented 2 weeks ago

Oh god..

Just stepping back for a moment here, this is all just so we can throw an exception if there's a stack overflow... Is it actually possible to throw a C++ exception when there's a stack overflow? Or is there another version of alloca on windows that returns an error value instead of throwing?

No, not interested in trying to throw an exception really. It might be possible but I don't see the point. This is just to keep the compilers happy

I was initially trying to avoid pragma ignores (as most of the choc codebase does) but I think that's the best way forward, what about something along these lines (untested), I think this closely mirrors what JUCE is doing.

static inline void* stackAllocate (size_t size)
{
   #if _MSC_VER
    #pragma warning push
    #pragma warning (disable: 6255 6386)
    return _alloca (size);
    #pragma warning pop
   #else
    return alloca (size);
   #endif
}
julianstorer commented 2 weeks ago

Can you call alloca inside a function and return the result? Unless the compiler definitely inlines the function, surely the memory would go out of scope at the end of the function?

Anthony-Nicholls commented 2 weeks ago

Can you call alloca inside a function and return the result? Unless the compiler definitely inlines the function, surely the memory would go out of scope at the end of the function?

haha nope! OK ignore the function that was me trying to be too clean and not thinking straight, but the rest is still relevant.

Anthony-Nicholls commented 2 weeks ago

I've got the code I've pushed here also building on JUCE CI I'll let you know how it goes.

julianstorer commented 2 weeks ago

OK, I can go with that

Anthony-Nicholls commented 2 weeks ago

OK all tests are passing now. It's only the last commit that actually matters, I'll let you decide if you want to do anything with the other two commits or not, they're not needed for JUCE at all.

julianstorer commented 2 weeks ago

Cheers. I think I'll leave the windows header thing for now. Although in general I always try to de-dupe code like that, in choc I also try to reduce the number of other choc files that each one depends on, so allow a little bit of code duplication to creep in for that.