On 03.06.25 12:25, Will Godfrey wrote:
I hasten to add I won't be attempting to get this in place for LAC!
Well... actually...
I couldn't resist at least having a quick look at this just to see
if it was at least viable and one thing led to another.
There is a branch, 'parts'. This is complete for the GUI
but not started yet for the CLI.
Hi Will,
what follows is a quick review of your branch.
Bottom line:
- like Kristian, I am much in favour of such an addition.
- the way you did it, looks lean and self-contained
- the topic "CLI" requires some further consideration.
I would vote for taking that branch in (after clean-up),
because it brings an immediate and obvious benefit.
...and I'd rather handle the topic of the CLI in a second step.
Review annotations
==================
(1) there is an unnecessary #include <bitset> in SynthEngine.h
(2) the `using std::bitset` in InterChange.h is unecessary;
we refer to this name exactly once, in the definition of the member field
and there the type is qualified with namespace "std" (which is adequate)
(3) the new field InterChange::partsChange is needlessly public.
Please make it private, to prevent any "convenient" grab at
that field from the outside. This helps to reduce complexity.
(4) a mistake was made with the (intended) initialisation of this new field
Actually, in the *body* of the constructor *a new in-function static*
variable is defined and initialised, which *shaddows* the member field
for the duration of the constructor body, since it has the same name
"partsChanged". Other than that, this statement has no effect.
Remedy: simply remove this line.
- std::bitset is a class with a default constructor, so it is already
initialised automatically to all-zero by default.
- it would be good practice though, to add that into the
member initialisation list of the constructor explicitly.
In the yoshimi code base, all larger objects have such an extended
member initialisation list and provde a fixed default value, even
for fields which are then dynamically initialised in some cases.
How to do that? simply add "partsChanged{}, " into that list,
and in the correct order resp. to the field definition, otherwise
the compiler will give a warning that the order of initialisation
will be in definition order of the fields, not in the order
listed in the constructor.
(5) at the one point where bits are actively set, in
InterChange::commandSendReal(), there is some additional
diagnostics code. Persumably this will be cleaned-up prior to release?
Regarding the topic of CLI: see the following Mail
-- Hermann
_______________________________________________
Yoshimi-devel mailing list
Yoshimi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/yoshimi-devel
