Address review: const ref args, revert lock migration

- TriggerForwarder::operator() now takes const Ts&... to avoid
  copies of non-trivial types (e.g., std::string).
- Revert lock to trigger-based pattern: LockStateForwarder holds
  two pointers (automation + lock), exceeding sizeof(void*) which
  would cause Callback::create to heap-allocate.
- Remove forwarder_extra_args from API since lock was the only user.
This commit is contained in:
J. Nick Koston
2026-03-25 15:10:10 -10:00
parent ee1da10614
commit bdc4a54613
5 changed files with 16 additions and 56 deletions
+5 -16
View File
@@ -672,7 +672,6 @@ async def build_callback_automation(
args: TemplateArgsType,
config: ConfigType,
forwarder: MockObjClass | None = None,
forwarder_extra_args: list | None = None,
) -> None:
"""Build an Automation and register it as a callback on the parent.
@@ -680,7 +679,9 @@ async def build_callback_automation(
automation's trigger() directly as a callback on the parent component.
Uses template forwarder structs so the compiler deduplicates the operator()
body across all call sites with the same signature.
body across all call sites with the same signature. The forwarder must be
pointer-sized (single Automation* field) to fit inline in Callback::ctx_
and avoid heap allocation.
:param parent: The component object (e.g., button, sensor).
:param callback_method: Name of the callback method (e.g., "add_on_press_callback").
@@ -688,11 +689,7 @@ async def build_callback_automation(
:param config: The automation config dict.
:param forwarder: Optional forwarder type to use instead of the default
TriggerForwarder<Ts...>. Pass any struct type whose aggregate init takes
an Automation pointer as the first field (e.g., TriggerOnTrueForwarder,
or a custom component-defined forwarder).
:param forwarder_extra_args: Optional list of extra MockObj args to pass to the
forwarder after the automation pointer in aggregate init. For example,
a lock forwarder needs the lock entity pointer: [lock_var].
a single Automation pointer (e.g., TriggerOnTrueForwarder).
"""
arg_types = [arg[0] for arg in args]
templ = cg.TemplateArguments(*arg_types)
@@ -708,12 +705,4 @@ async def build_callback_automation(
if arg_types
else TriggerForwarder.template()
)
init_args = str(obj)
if forwarder_extra_args:
extra = ", ".join(str(a) for a in forwarder_extra_args)
init_args = f"{init_args}, {extra}"
cg.add(
getattr(parent, callback_method)(
cg.RawExpression(f"{forwarder}{{{init_args}}}")
)
)
cg.add(getattr(parent, callback_method)(cg.RawExpression(f"{forwarder}{{{obj}}}")))
+4 -17
View File
@@ -35,7 +35,6 @@ LockLockTrigger = lock_ns.class_("LockLockTrigger", automation.Trigger.template(
LockUnlockTrigger = lock_ns.class_("LockUnlockTrigger", automation.Trigger.template())
LockState = lock_ns.enum("LockState")
LockStateForwarder = lock_ns.class_("LockStateForwarder")
LOCK_STATES = {
"LOCKED": LockState.LOCK_STATE_LOCKED,
@@ -95,23 +94,11 @@ def lock_schema(
@setup_entity("lock")
async def _setup_lock_core(var, config):
for conf in config.get(CONF_ON_LOCK, []):
await automation.build_callback_automation(
var,
"add_on_state_callback",
[],
conf,
forwarder=LockStateForwarder.template(LockState.LOCK_STATE_LOCKED),
forwarder_extra_args=[var],
)
trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var)
await automation.build_automation(trigger, [], conf)
for conf in config.get(CONF_ON_UNLOCK, []):
await automation.build_callback_automation(
var,
"add_on_state_callback",
[],
conf,
forwarder=LockStateForwarder.template(LockState.LOCK_STATE_UNLOCKED),
forwarder_extra_args=[var],
)
trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var)
await automation.build_automation(trigger, [], conf)
if mqtt_id := config.get(CONF_MQTT_ID):
mqtt_ = cg.new_Pvariable(mqtt_id, var)
-10
View File
@@ -66,14 +66,4 @@ template<LockState State> class LockStateTrigger : public Trigger<> {
using LockLockTrigger = LockStateTrigger<LockState::LOCK_STATE_LOCKED>;
using LockUnlockTrigger = LockStateTrigger<LockState::LOCK_STATE_UNLOCKED>;
/// Forwarder that triggers an Automation<> when a Lock reaches a specific state.
template<LockState State> struct LockStateForwarder {
Automation<> *automation;
Lock *lock;
void operator()() const {
if (this->lock->state == State)
this->automation->trigger();
}
};
} // namespace esphome::lock
+1 -1
View File
@@ -495,7 +495,7 @@ template<typename... Ts> class Automation {
/// One operator() instantiation per Automation<Ts...> signature, shared across all call sites.
template<typename... Ts> struct TriggerForwarder {
Automation<Ts...> *automation;
void operator()(Ts... args) const { this->automation->trigger(args...); }
void operator()(const Ts &...args) const { this->automation->trigger(args...); }
};
/// Callback forwarder that triggers an Automation<> only when the bool arg is true.
+6 -12
View File
@@ -187,7 +187,6 @@ def _build_forwarder(
automation_name: str,
args: list[tuple[str, str]],
forwarder: MockObj | None = None,
extra_args: list[str] | None = None,
) -> str:
"""Build a trigger forwarder expression the same way build_callback_automation does.
@@ -202,10 +201,7 @@ def _build_forwarder(
cg.TemplateArguments(*arg_types) if arg_types else cg.TemplateArguments()
)
forwarder = TriggerForwarder.template(templ)
init_args = str(obj)
if extra_args:
init_args += ", " + ", ".join(extra_args)
return f"{forwarder}{{{init_args}}}"
return f"{forwarder}{{{obj}}}"
def test_trigger_forwarder_no_args() -> None:
@@ -253,10 +249,8 @@ def test_trigger_forwarder_string_arg() -> None:
assert result == "TriggerForwarder<std::string>{auto_1}"
def test_trigger_forwarder_custom_with_extra_args() -> None:
"""Lock on_lock: custom forwarder with extra args for entity pointer."""
lock_forwarder = MockObj("LockStateForwarder<LOCK_STATE_LOCKED>", "")
result = _build_forwarder(
"auto_1", [], forwarder=lock_forwarder, extra_args=["lock_var"]
)
assert result == "LockStateForwarder<LOCK_STATE_LOCKED>{auto_1, lock_var}"
def test_trigger_forwarder_custom_type() -> None:
"""Custom forwarder type passed directly."""
custom = MockObj("MyForwarder", "")
result = _build_forwarder("auto_1", [], forwarder=custom)
assert result == "MyForwarder{auto_1}"