[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Issue 5362: Generalize context searches (issue 344050043 by address@
From: |
dak |
Subject: |
Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden) |
Date: |
Sun, 01 Jul 2018 10:14:25 -0700 |
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/
- Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden), dak, 2018/07/01
- Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden), dak, 2018/07/01
- Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden), nine . fierce . ballads, 2018/07/01
- Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden), nine . fierce . ballads, 2018/07/01
- Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden),
dak <=
- Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden), dak, 2018/07/01
- Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden), dak, 2018/07/01
- Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden), dak, 2018/07/02
- Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden), nine . fierce . ballads, 2018/07/02