John Snow <js...@redhat.com> writes:
> This is a silly one, but... it's important to have fun.
>
> This patch isn't *needed*, it's here as an RFC. In trying to experiment
> with different ways to solve the problem addressed by the previous
> commit, I kept getting confused at how the "source location" string with
> line and column number was built across two different classes.
>
> (i.e. QAPISourceError appends the column, but QAPISourceInfo does not
> track column information natively.)
>
> I was afraid to try and fully implement column number directly in
> QAPISourceInfo on the chance that it might have undesirable effects, so
> I came up with a quick "hack" to centralize the 'location' information
> generation.
>
> It's a little goofy, but it works :')
>
> Signed-off-by: John Snow <js...@redhat.com>
> ---
> scripts/qapi/error.py | 8 +++-----
> scripts/qapi/source.py | 23 ++++++++++++++++++++++-
> 2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
> index e35e4ddb26a..6b04f56f8a2 100644
> --- a/scripts/qapi/error.py
> +++ b/scripts/qapi/error.py
> @@ -39,11 +39,9 @@ def __init__(self,
>
> def __str__(self) -> str:
> assert self.info is not None
> - loc = str(self.info)
> - if self.col is not None:
> - assert self.info.line is not None
> - loc += ':%s' % self.col
> - return loc + ': ' + self.msg
> + with self.info.at_column(self.col):
> + loc = str(self.info)
> + return f"{loc}: {self.msg}"
>
>
> class QAPISemError(QAPISourceError):
> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> index 1ade864d7b9..21090b9fe78 100644
> --- a/scripts/qapi/source.py
> +++ b/scripts/qapi/source.py
> @@ -9,8 +9,14 @@
> # This work is licensed under the terms of the GNU GPL, version 2.
> # See the COPYING file in the top-level directory.
>
> +from contextlib import contextmanager
> import copy
> -from typing import List, Optional, TypeVar
> +from typing import (
> + Iterator,
> + List,
> + Optional,
> + TypeVar,
> +)
>
>
> class QAPISchemaPragma:
> @@ -35,6 +41,7 @@ def __init__(self, fname: str, line: int,
> parent: Optional['QAPISourceInfo']):
> self.fname = fname
> self.line = line
> + self._column: Optional[int] = None
> self.parent = parent
> self.pragma: QAPISchemaPragma = (
> parent.pragma if parent else QAPISchemaPragma()
> @@ -52,9 +59,14 @@ def next_line(self: T) -> T:
> return info
>
> def loc(self) -> str:
> + # column cannot be provided meaningfully when line is absent.
> + assert self.line or self._column is None
> +
> ret = self.fname
> if self.line is not None:
> ret += ':%d' % self.line
> + if self._column is not None:
> + ret += ':%d' % self._column
> return ret
>
> def in_defn(self) -> str:
> @@ -71,5 +83,14 @@ def include_path(self) -> str:
> parent = parent.parent
> return ret
>
> + @contextmanager
> + def at_column(self, column: Optional[int]) -> Iterator[None]:
> + current_column = self._column
> + try:
> + self._column = column
> + yield
> + finally:
> + self._column = current_column
> +
> def __str__(self) -> str:
> return self.include_path() + self.in_defn() + self.loc()
Uh...
We create one QAPISourceInfo instance per source line. First line in
QAPISchemaParser.__init__()
self.info = QAPISourceInfo(fname, 1, incl_info)
and subsequent ones in .accept()
self.info = self.info.next_line()
These instances get shared by everything on their line.
Your patch adds a _column attribute to these objects. Because it
applies to everything that shares the object, setting it is kind of
wrong. You therefore only ever set it temporarily, in
QAPISourceError.__str__().
This works in the absence of concurrency (which means it just works,
this being Python), but *ugh*!
The obvious extension of QAPISourceInfo to columns would create one
instance per source character. Too egregiously wasteful for my taste.
Let's start over with the *why*: what is it exactly that bothers you in
the code before this patch? Is it the spatial distance between the
formatting of "file:line:col: msg" in QAPISourceError.__str_() and the
formatting of the file:line part in QAPISourceInfo.loc()?
