| Message ID | 20210422030720.3685766-3-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt5a | expand |
John Snow <jsnow@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 <jsnow@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()?
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()
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 <jsnow@redhat.com> --- scripts/qapi/error.py | 8 +++----- scripts/qapi/source.py | 23 ++++++++++++++++++++++- 2 files changed, 25 insertions(+), 6 deletions(-)