In juce_AudioProcessor.cpp, line 900 (as of the last time I puled the code) contains the following seemingly innocuous statement
jassert (hasEditor() == (ed != nullptr));
However, some implementations of hasEditor(), for example the version in juce_VST3PluginFormat.cpp, starting at line 2779 actually create an editor if one does not exist.
In other words, the call to hasEditor() changes state, i.e, there are side effects.
An assert statement must never invoke anything that can cause side effects since the assert will disappear when the code is compiled in non-debug mode and if something depends on that side effect, then the code will break just by non-debug compilation.
On a cursory examination of the code, it probably makes sense to change that assert to just
jassert(getActiveEditor() != nullptr);
(I am of course assuming that getActiveEditor() doesnât in fact try to create an editor
I would rather question the implementation in the juce_VST3PluginFormat.cpp.
bool hasEditor() is a pure abstract that is simply supposed to return true or false:
Your processor subclass must override this and return true if it can create an editor component.
The implementation you quoted is from AudioPluginInstance, which is the hosting counterpart.
It seems the code creates an editor to find out, if one can be constructed. I donât know the API well enough to answer, if there is a way to check without creating an editor.
I agree that "hasEditor()* should not create an editor if one isnât there. Thatâs certainly the expected behavior based on the comment for the base abstract method.
One question is whether correcting that code for the VST3 if it can be corrected (see below) would break existing code that depends on that functionality.
We certainly did not expect an editor to be created (it doesnât happen for the VST2) and thatâs how we discovered the problem.
Yep - I donât know why theyâre doing that for VST3 unless thereâs something funky about the VST3 SDK such that there is no way to find out if a plugin support an editor without trying to actually create one. But in that case, the (temporarily?) created editor should be immediately destroy.
Indeed - but either the VST3 code or the assertion needs to be fixed.
It looks to me like this code is trying to guard against the case where IEditController::createView might return nullptr. I canât see any other way on the IEditController interface to check whether or not the edit controller can create an IPlugView.
Iâm pretty sure this is what happens:
bool hasEditor() const override
{
// (if possible, avoid creating a second instance of the editor, because that crashes some plugins)
if (getActiveEditor() != nullptr)
return true;
VSTComSmartPtr<IPlugView> view (tryCreatingView(), false);
return view != nullptr;
}
The VSTComSmartPtr will call release on the view when it goes out of scope, destroying it.
So, while this assertion has side-effects, Iâm not sure that they are truly dangerous side-effects. i.e. the side effects do not change the state of the AudioProcessor, and do not call any functions on the IEditController that it does not support (I think itâs valid to create multiple IPluginView instances simultaneously).
Could easily remove the potential danger by storing the result of hasEditor() in a local temporary before asserting on it? Rather than needing to change the logic of the assertion itself
Isnât the issue that hasEditor() can have side-effects and so the behaviour could be different between a debug build and release build when itâs only called from within the assertion?
If you pull out the call to hasEditor() then itâll always be called, removing the potential risk of different behaviours?
Like @reuk pointed out before, the wrapper creates an editor that is removed immediately after.
So you can argue if creating a local instance and discard it is really a side effect.
That would qualify a lot of functions as having side-effects.
That is precisely the issue and CS 101 ânever ever invoke a function with side effects from an assertâ because different build processes will result in different semantics. Thatâs just a no-no â surprised this is even up for discussion.
Stop right there â itâs just damn scary to see this kind of claim.
Do you not understand that if you do this, then when the code is compiled in release mode, there will be no attempt to invoke that (temporary) creation in the first place? Just because you think this is harmless doesnât mean it is â in fact this broke our code because of something we were intentionally doing .
There are no circumstances where invoking an assertion on a function that has side effects is legitimate, no matter how harmless someone might think are the side effects.
Side effects are often fine in assertions. For example, allocating and freeing memory might be considered a side effect (youâre modifying the state of the system), but itâs quite reasonable to copy a string or some other data structure in the condition of an assertion. The question is whether the side effect (or absence thereof) changes the semantics of the program in a way that introduces breakage.
In this case, Iâd argue that the program is valid whether or not the assertion is present, because the VST3 API is being used correctly in both cases. If a particular plugin depends on the assertion being there (or not being there) then that would indicate an issue in the implementation of the plugin.
NO - it is not at all reasonable â it is never reasonable.
I quote from a post in Stack Overflow:
We have several moderately sized C code bases that receive commits from developers with a variety of experience levels. Some of the less disciplined programmers commit assert() statements with side effects that cause bugs with assertions disabled. E.g.*
You have absolutely no way to be able to prove that such an operation would be guaranteed safe and to have zero impact based on compilation. The very act of copying can change something - even a ref count in a string â and something else might be depending on that ref count - even just for the purpose of debugging some other issue.
I do not want a plugin editor to be created when running debug version but not to be created when running release version. You have absolutely no idea what some particular plugin editor might do when it is just created that is undesirable under certain circumstances (or the reverse â you might inadvertently be depending on that plugin editor to have been created and then youâll have a problem when you build in release mode and your code instantly breaks)
Thank goodness we have the source - weâll fix this ourselves â but with respect, you have a responsibility to ensure that your code base is correct as far as is possible and an assertion with side effects is never correct.
The âoften fineâ approach is typically not a great rule to use in programming generally. The assertions specifically are there to catch the âodd/unexpected behaviorâ so it has to work all the time, not most of the time.
Take this exact example from this post this example - itâs fine until it isnât and you end up creating VST3 views without knowing it and, whatâs infinitely worse - only in the debug build.
So the philosophy of something being fine 99.9% typically doesnât fare well as a general programming rule.
I think the main real problem is that hasEditor actually has side effects. The name and signature (const) implies, that it doesnât have side affects (and one might also argue that from the documentation one should expect the method to run in constant fast runtime). This seems to be not the case.
I think from a software design approach you should mark this method as deprecated and introduce a new one that does not imply having no side affects if there is no way around the current implementation for VST3 plugins.
Iâm also interested in how this even compiles. If @dhjdhj experienced an actual change of behaviour clearly there is a method const that shouldnât have been made const.
In either of those two situations it means that the plug-in has behaviour outside what is expected of the plug-in specification. This problem is getting flagged up now, but thereâs a decent chance it might also manifest itself elsewhere if the plug-in is being hosted by a specific DAW.
It never occurred to me that I would have to argue about how an assert statement works.
The JUCE developers can certainly decide whether the fix should be to hasEditor() method or to the assert statement. Both of these were written by the JUCE developers (I assume).
All I wanted to do was point out that as it stands, thereâs a bug!
A basic âhello worldâ program that includes those modules would compile. To paraphrase Galileo, itâs still wrong
You might have missed my point. I think the const-correctness in C++ should have prevented this bug to even appear.
When you just remove it, how exactly can you know that the assertion is not added again in a few years by someone else? I would find it extremely frustrating correcting a bug with no check/test whatsoever that protects me from making it again.
Changing the hasEditor method would have the benefit to find a possible related bug.
But could you provide us with the actual steps to reproduce the bug?
While I understand the criticism from a formal point of view here, Iâm still struggling to understand why this certain assertion might actually cause any trouble in a standard conforming real-world plugin.
Given the fact that a plugin should be able to just create and destroy its editor at any time point in order to fulfil the requirements of the common plugin standards and that also common plugin hosts out there will do exactly that when instantiating a plugin for the first time, I would tend to say that any plugin that experiences real problems here is itself not standard conforming and that this only brings up a design issue in the plugin itself.
I think it would be extremely beneficial for this discussion if you provided quite a bit more details of your actual use-case @dhjdhj and how creating and destroying an editor from inside an assertion is different to your plugin from reacting to the same behaviour from a plugin host in the wild?
