Remove unused Namespace#m_ConstValues #9656
Conversation
lib/base/namespace.cpp
Outdated
| Namespace::Namespace(bool constValues) | ||
| : m_ConstValues(constValues), m_Frozen(false) | ||
| : m_Frozen(false) | ||
| { } |
There was a problem hiding this comment.
This makes the constructor parameter unused. There are still places that set it. Now I'm not 100% sure that 1c066fc removing the use of m_ConstValues couldn't have introduced a bug.
In general, I found the behavior of constValues quite strange, so I won't be sad if it's gone, quite the opposite. In general, it made sure that everything that was inserted into the namespace was made const. Can you please check if that's ensured by other means now for all places that still set it to true? By other means should mean all inserts explicitly set the values as const and the namespace is frozen so that users can't insert non-const values. If that's the case, please remove the parameter as well.
There was a problem hiding this comment.
Have a look at the just pushed commit. All affected namespaces get frozen except Internal and NamespaceExpression (DSL). I read your comment like at least the latter makes m_ConstValues actually necessary.
There was a problem hiding this comment.
Indeed, there was an unintended change in behavior:
Latest release:
Icinga 2 (version: v2.13.6)
Type $help to view available commands.
<1> => namespace X { a = 23; a = 42; }
^^^^^^
Constant must not be modified.
Current master:
Icinga 2 (version: 14d7ee2)
Type $help to view available commands.
<1> => namespace X { a = 23; a = 42; }
null
Good to know that there was a change, it doesn't look super bad though. Also the behavior in the current release is somewhat inconsistent:
Icinga 2 (version: v2.13.6)
Type $help to view available commands.
<1> => X = Namespace(); X.a = 23; X.a = 42
null
Also, the globals namespace in the released version behaves completely different than other namespaces (there you can additionally use the const keyword to alter the behavior, note that the deprecation now also results in an error on master):
Icinga 2 (version: v2.13.6)
Type $help to view available commands.
<1> => globals.x = 23
null
<2> => globals.x = 42
null
<3> => const y = 23
null
<4> => const y = 42
warning/Value for constant 'y' was modified. This behaviour is deprecated.
Location: in <4>: 1:0-1:11:
null
<5> => y = 1337
^^^^^^^^
Constant must not be modified.
So in general, the behavior looks weird enough that I'd say we can simplify stuff here as long as it doesn't introduce unnecessary incompatibility. Thus, making all namespaces append-only (that could be a better name for constValues) probably isn't an option as you couldn't reassign globals and as it would make the const keyword pointless. The only downside I see is that it wouldn't error out if you accidentally reassign something in a namespace (i.e. you see the current error as a feature) but that's already not the case in the globals namespace. (I really nobody attempts this in try/except and depends on it throwing an error.)
tl;dr: yes, there already happened a change in behavior, but so far I don't think that's to bad and we might declare it a feature (but let me sleep about that first)
No description provided.