Re: Patch: debug-instrumented predicate

[Arthur Miller]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Arthur Miller [2021-10-04 23:58:26] wrote:
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>> Concrete use-case would be to offer a user some kind of gui to instrument 
>>>> or remove
>>>> instrumentation for debug/edebug/profile/trace.
>>>
>>> W.r.t *removal* of debug/profile/trace, this could be done by offering
>>> a generic removal GUI for advice.
>>
>> Njah, advice already has add/remove which can be connected to a gui button or
>> what not. However any generic interface always need a specialisation for a 
>> case
>> at hand anyway, at least a different label on a button. But we are drifting
>> away.
>
> I don't think it's drifting: I'm suggesting that we try to add support
> for it in a way that supports them all at the same time.
>
>> I ask/suggest for an API to discover if instrumentation is installed or not.
>
> And I'm suggesting that make `advice-member-p` should be the way to do that.
> Currently this requires reliance on some internal knowledge of the
> advice's name, but we could fix it by making this name public&stable.
Allright, sorry than :).

Yes, sure, that would work of course. Personally I would still prefer if you
choose to use a function to expose as API rather than the advice name. Even if
it is just a tiny wrapper around advice-member-p, such as trace-is-traced or
elp--instrumented-p. I personally think it would be more convenient. And also,
probably anyone interested to use this functionality is goingt tomake a wrapper
just like trace and elp author(s) did :).

>> The only one that truly is missing a mean to test for on/off state is edebug.
>
> Indeed.  I haven't looked at whether it could be made to use
> `advice-add` so that `advice-member-p` could be used as well.
> There's a good chance that it's not really an option (I mean
> technically, I'm pretty sure it could be done, but I suspect it will
> come with enough downsides that it's not worth it).

Edebugg could add a tiny advice that really does nothing, but just serves
as a flag to signal that a function is instrumented. It adds a function
call extra as overhead, but I guess, we are debugging anyway, so it shouldn't
matter?

Otherwise they could just put something into symbol's plist and remove it. That
eliminates the cost of a function call.

> Until it can be made to use `advice-member-p` we should indeed export
> a public way to test whether a function is instrumented by Edebug.
>
>> I don't know which one is more efficient, but I don't like neither of those 
>> two,
>> I don't think any of suggested solutions is efficient. Also they both rely on
>> internal state that can change whenever.
>
> As long as these functions are defined in `edebug.el` it's OK if they
> rely on internal state.

Well, yes I guess; that is why I sent in those three patches, I don't want to
have it in my own (external) package.

I also would like to have *-instrumented-p as API, I think it is
self-documenting what it means; but that might be just me.

>> When a function is instrumented for edebug, edebug adds some properties into
>> symbols property list. However when instrumentation is removed, edebug leaves
>> those properties to scrap behind; that inclusive the position in buffer 
>> where it
>> was active. IMO it is a bug; it shouldn't leave scrap behind. It would also 
>> be
>> trivial to check if a funciton is instrumented or not, if those properties 
>> were
>> removed when instrumentation is removed.
>>
>> Unless I misunderstand the purpose why properties are left behind after
>> instrumentation is removed :).
>
> I suspect they're not left behind for any good reason, and
> the best way to find out is to remove them and see if someone screams.

Ok, will you remove it or do you want a patch? Same for the discussion above;
are you doing those changes yourself or is there something I can do?