[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel

From: Markus Armbruster
Subject: Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
Date: Wed, 13 Jan 2021 16:39:15 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)

Spelling nitpick: s/builtin/built-in/ in the title.

John Snow <jsnow@redhat.com> writes:

> We use None to represent an object that has no source information
> because it's a builtin. This complicates interface typing, since many
> interfaces expect that there is an info object available to print errors
> with.
> Introduce a special QAPISourceInfo that represents these built-ins so
> that if an error should so happen to occur relating to one of these
> builtins that we will be able to print its information, and interface
> typing becomes simpler: you will always have a source info object.
> This object will evaluate as False, so "if info" remains a valid
> idiomatic construct.
> NB: It was intentional to not allow empty constructors or similar to
> create "empty" source info objects; callers must explicitly invoke
> 'builtin()' to pro-actively opt into using the sentinel. This should
> prevent use-by-accident.
> Signed-off-by: John Snow <jsnow@redhat.com>

As I pointed out in review of v1, this patch has two aspects mixed up:

1. Represent "no source info" as special QAPISourceInfo instead of

2. On error with "no source info", don't crash.

The first one is what de-complicates interface typing.  It's clearly
serving this patch series' stated purpose: "static typing conversion".

The second one is not.  It sidetracks us into a design discussion that
isn't related to static typing.  Maybe it's something we should discuss.
Maybe the discussion will make us conclude we want to do this.  But
letting the static typing work get delayed by that discussion would be
stupid, and I'll do what I can to prevent that.

The stupidest possible solution that preserves the crash is adding an
assertion right where it crashes before this patch: in
QAPISourceInfo.__str__().  Yes, crashing in a __str__() method is not
nice, but it's no worse than before.  Making it better than before is a
good idea, and you're quite welcome to try, but please not in this
series.  Add a TODO comment asking for "make it better", then sit on
your hands.

The pipeline must move.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]