kicad-developers team mailing list archive

[John Beard <john.j.beard@xxxxxxxxx>]

On 29/04/2019 04:25, Jon Evans wrote:> Is there any particular 
philosophical or technical reason why
> ADVANCED_CONFIG was created as a read-only system (i.e. to modify any
> values, you are supposed to edit the text config file and restart the
> program)?

More of a philosophical reason.

Mostly because the ADVANCED_CONFIG object has one job, and one job only: 
presenting a simple (type safe) view of what advanced config is present, 
and providing suitable defaults if not, while hiding the implementation 
(a file).

It's a singleton because it's designed to be very simple to access and 
use. Singletons are (generally) bad news as they transgress normal 
expectations of concurrency, ownership and access. This is why it's 
const, to ensure 1) it's always consistent and 2) to avoid use as a 
general-purpose about:config-style read/write store for random bits and 
bobs (which leads to risky changes to global state that A_C is not 
designed to address).

It's not expected to change at runtime (if it's supposed to be 
changeable by the user, then it's not advanced config, it's just config, 
and it should have proper UI).

Co-opting advanced config for storing domain-specific state that comes 
from internal processes far away in the code seems a little bit mushy in 
terms of separation of concerns.

Doubly so in the case of things that might change over time. Then you 
might start running into consistency issues with multiple users of A_C
getting different answers. If clients want to change their own 
parameters, they should implement their own class invariants, where A_C 
is just one of the inputs to the decision.

> (The use case is that real-time connectivity is currently an advanced
> config that defaults to off.  I would like to change it so that
> real-time is defaulted to on, but can be disabled in advanced config,
> and can also be disabled if an internal performance threshold trips.
> The easiest way to do this would be to modify the state of the
> ADVANCED_CONFIG singleton at runtime.)

I suggest having an advanced config "Disable Realtime Connectivity" as 
the override and combine this with the internal perf threshold at the 
point of use:

E.g. somewhere suitable, compute (and cache as needed) this value:

bool isRealTime = !ADVANCED_CFG::GetCfg().m_realTimeConnectivityDisabled 
&& connectivityPerfOKForRealTime();

Then if you need to change this value, you can implement your own 
mechanisms for ensuring all interested code has the right value of 
isRealTime (mutex, signal, event, etc etc) whenever the domain-local 
conditions change.

Cheers,

John