Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden)

[dak]

On 2018/07/01 16:48:09, Dan Eble wrote:

https://codereview.appspot.com/344050043/diff/20001/lily/change-iterator.cc
> File lily/change-iterator.cc (right):


https://codereview.appspot.com/344050043/diff/20001/lily/change-iterator.cc#newcode67
> lily/change-iterator.cc:67: Context::warning_cannot_find (origin,
to_type,
> to_id);
> On 2018/07/01 13:21:43, dak wrote:
> > Having a separate function for each different warning text seems
clumsy.  Can
> > this be factored into a more generic warning function and possibly
something
> > formatting to_type and to_id?

> I'm willing to change this, but it's not perfectly clear to me what
you are
> asking for.  (The only change I would object to is pushing the logging
back into
> the find/create functions, and you're obviously not asking for that.)

> Would you prefer a generic message saying "cannot find or create ..."
even when
> only one has been tried?  Would you prefer that the caller provide the
mode as a
> function parameter?

> I am also having trouble imagining what additional formatting of
to_type and
> to_id you are thinking of.  One is a symbol and one is a string, and
the warning
> functions print them as strings.  If you mean you want the caller to
provide
> some formatting, I would prefer to leave Context in control of the
formatting to
> help maintain consistency.

Something like
origin->warning (_f ("Cannot find or create context: %s",
ctx->identification ()))

Well, in this case ctx could not created, so we need an alternative
static string Context::identification (id, name)
that would be used internally of string Context::identification () const
assuming that one can overload static and non-static functions.  If not,
have a context_identification (id, name) instead.

That way, the warning text is directly where it gets produced, and the
context-identification formatter might be nice even from Scheme, with
either a context argument or two arguments (whatever it was, symbol or
string).  That way we'd get a uniform format for such messages.

https://codereview.appspot.com/344050043/