| Message ID | 20210729012311.1406847-2-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | df82de805172687e88dd7d72b68a9223b0a4c269 |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | dtoc: Improve support for 'ranges' property | expand |
The current name is confusing because the logic is actually backwards from
what you might expect. Rename it to needs_widening() and update the
comments.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
tools/dtoc/fdt.py | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
Applied to u-boot-dm, thanks!
Hi Simon, Thanks for checking this bug, I'm glad that you were able to come with fix quickly. I have some questions, I hope that you find some time to help me understand. On 7/28/21 10:23 PM, Simon Glass wrote: > The current name is confusing because the logic is actually backwards from > what you might expect. Rename it to needs_widening() and update the > comments. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > tools/dtoc/fdt.py | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py > index 3996971e39c..9749966d5fb 100644 > --- a/tools/dtoc/fdt.py > +++ b/tools/dtoc/fdt.py > @@ -24,16 +24,19 @@ from patman import tools > > # A list of types we support > class Type(IntEnum): > + # Types in order from widest to narrowest > (BYTE, INT, STRING, BOOL, INT64) = range(5) Sorry but I don't understand why BYTE is wider than INT (or INT64) > > - def is_wider_than(self, other): > - """Check if another type is 'wider' than this one > + def needs_widening(self, other): > + """Check if this type needs widening to hold a value from another type > > - A wider type is one that holds more information than an earlier one, > - similar to the concept of type-widening in C. > + A wider type is one that can hold a wider array of information than > + another one, or is less restrictive, so it can hold the information of > + another type as well as its own. This is similar to the concept of > + type-widening in C. > > This uses a simple arithmetic comparison, since type values are in order > - from narrowest (BYTE) to widest (INT64). > + from widest (BYTE) to narrowest (INT64). > > Args: > other: Other type to compare against > @@ -149,7 +152,7 @@ class Prop: > update the current property to be like the second, since it is less > specific. > """ > - if self.type.is_wider_than(newprop.type): > + if self.type.needs_widening(newprop.type): > if self.type == Type.INT and newprop.type == Type.BYTE: > if type(self.value) == list: > new_value = [] Regards, Walter
Hi Walter, On Sun, 1 Aug 2021 at 20:45, Walter Lozano <wlozano@collabora.com> wrote: > > Hi Simon, > > Thanks for checking this bug, I'm glad that you were able to come with > fix quickly. I have some questions, I hope that you find some time to > help me understand. > > On 7/28/21 10:23 PM, Simon Glass wrote: > > The current name is confusing because the logic is actually backwards from > > what you might expect. Rename it to needs_widening() and update the > > comments. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > tools/dtoc/fdt.py | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py > > index 3996971e39c..9749966d5fb 100644 > > --- a/tools/dtoc/fdt.py > > +++ b/tools/dtoc/fdt.py > > @@ -24,16 +24,19 @@ from patman import tools > > > > # A list of types we support > > class Type(IntEnum): > > + # Types in order from widest to narrowest > > (BYTE, INT, STRING, BOOL, INT64) = range(5) > > Sorry but I don't understand why BYTE is wider than INT (or INT64) I think perhaps we need a better name. A wider type is one that can hold the values of a narrower one, plus more. In this case a 'bytes' type can hold anything (bytes, int, int64, bool) so is the 'widest' there is. It is the lowest common denominator in the devicetree. > > > > > - def is_wider_than(self, other): > > - """Check if another type is 'wider' than this one > > + def needs_widening(self, other): > > + """Check if this type needs widening to hold a value from another type > > > > - A wider type is one that holds more information than an earlier one, > > - similar to the concept of type-widening in C. > > + A wider type is one that can hold a wider array of information than > > + another one, or is less restrictive, so it can hold the information of > > + another type as well as its own. This is similar to the concept of > > + type-widening in C. > > > > This uses a simple arithmetic comparison, since type values are in order > > - from narrowest (BYTE) to widest (INT64). > > + from widest (BYTE) to narrowest (INT64). > > > > Args: > > other: Other type to compare against > > @@ -149,7 +152,7 @@ class Prop: > > update the current property to be like the second, since it is less > > specific. > > """ > > - if self.type.is_wider_than(newprop.type): > > + if self.type.needs_widening(newprop.type): > > if self.type == Type.INT and newprop.type == Type.BYTE: > > if type(self.value) == list: > > new_value = [] > Regards, Simon
Hi Simon, On 8/1/21 11:50 PM, Simon Glass wrote: > Hi Walter, > > On Sun, 1 Aug 2021 at 20:45, Walter Lozano <wlozano@collabora.com> wrote: >> Hi Simon, >> >> Thanks for checking this bug, I'm glad that you were able to come with >> fix quickly. I have some questions, I hope that you find some time to >> help me understand. >> >> On 7/28/21 10:23 PM, Simon Glass wrote: >>> The current name is confusing because the logic is actually backwards from >>> what you might expect. Rename it to needs_widening() and update the >>> comments. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> tools/dtoc/fdt.py | 15 +++++++++------ >>> 1 file changed, 9 insertions(+), 6 deletions(-) >>> >>> diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py >>> index 3996971e39c..9749966d5fb 100644 >>> --- a/tools/dtoc/fdt.py >>> +++ b/tools/dtoc/fdt.py >>> @@ -24,16 +24,19 @@ from patman import tools >>> >>> # A list of types we support >>> class Type(IntEnum): >>> + # Types in order from widest to narrowest >>> (BYTE, INT, STRING, BOOL, INT64) = range(5) >> Sorry but I don't understand why BYTE is wider than INT (or INT64) > I think perhaps we need a better name. A wider type is one that can > hold the values of a narrower one, plus more. > > In this case a 'bytes' type can hold anything (bytes, int, int64, > bool) so is the 'widest' there is. It is the lowest common denominator > in the devicetree. Thanks for taking the time to explain. I understand the idea behind your explanation but I still not sure if I follow you completely. In any case, let me add a few words in order to be more clear. It is my impression that when you say 'bytes' (and not BYTE like in the declaration) you are referring to a list. Is that the case? If not, BYTE (8 bit) seems to be narrower than INT (32 bits), isn't it? Also, another example is INT, BOOL and INT64. It is clear that INT is wider than BOOL, but why BOOL is wider than INT64? As reference I have been checking https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#property-values >>> - def is_wider_than(self, other): >>> - """Check if another type is 'wider' than this one >>> + def needs_widening(self, other): >>> + """Check if this type needs widening to hold a value from another type >>> >>> - A wider type is one that holds more information than an earlier one, >>> - similar to the concept of type-widening in C. >>> + A wider type is one that can hold a wider array of information than >>> + another one, or is less restrictive, so it can hold the information of >>> + another type as well as its own. This is similar to the concept of >>> + type-widening in C. >>> >>> This uses a simple arithmetic comparison, since type values are in order >>> - from narrowest (BYTE) to widest (INT64). >>> + from widest (BYTE) to narrowest (INT64). >>> >>> Args: >>> other: Other type to compare against >>> @@ -149,7 +152,7 @@ class Prop: >>> update the current property to be like the second, since it is less >>> specific. >>> """ >>> - if self.type.is_wider_than(newprop.type): >>> + if self.type.needs_widening(newprop.type): >>> if self.type == Type.INT and newprop.type == Type.BYTE: >>> if type(self.value) == list: >>> new_value = [] > Regards, > Simon Regards, Walter
Hi Walter, On Mon, 2 Aug 2021 at 13:29, Walter Lozano <wlozano@collabora.com> wrote: > > Hi Simon, > > On 8/1/21 11:50 PM, Simon Glass wrote: > > Hi Walter, > > > > On Sun, 1 Aug 2021 at 20:45, Walter Lozano <wlozano@collabora.com> wrote: > >> Hi Simon, > >> > >> Thanks for checking this bug, I'm glad that you were able to come with > >> fix quickly. I have some questions, I hope that you find some time to > >> help me understand. > >> > >> On 7/28/21 10:23 PM, Simon Glass wrote: > >>> The current name is confusing because the logic is actually backwards from > >>> what you might expect. Rename it to needs_widening() and update the > >>> comments. > >>> > >>> Signed-off-by: Simon Glass <sjg@chromium.org> > >>> --- > >>> > >>> tools/dtoc/fdt.py | 15 +++++++++------ > >>> 1 file changed, 9 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py > >>> index 3996971e39c..9749966d5fb 100644 > >>> --- a/tools/dtoc/fdt.py > >>> +++ b/tools/dtoc/fdt.py > >>> @@ -24,16 +24,19 @@ from patman import tools > >>> > >>> # A list of types we support > >>> class Type(IntEnum): > >>> + # Types in order from widest to narrowest > >>> (BYTE, INT, STRING, BOOL, INT64) = range(5) > >> Sorry but I don't understand why BYTE is wider than INT (or INT64) > > I think perhaps we need a better name. A wider type is one that can > > hold the values of a narrower one, plus more. > > > > In this case a 'bytes' type can hold anything (bytes, int, int64, > > bool) so is the 'widest' there is. It is the lowest common denominator > > in the devicetree. > > Thanks for taking the time to explain. I understand the idea behind your > explanation but I still not sure if I follow you completely. In any > case, let me add a few words in order to be more clear. > > It is my impression that when you say 'bytes' (and not BYTE like in the > declaration) you are referring to a list. Is that the case? > > If not, BYTE (8 bit) seems to be narrower than INT (32 bits), isn't it? > A bit long in finding this email again... Actually bytes means the Python bytes type which holds a sequence of bytes. So it can hold an int, whereas an int cannot hold a bytes value, in general (although it could if it happened to be 4 bytes). > Also, another example is INT, BOOL and INT64. It is clear that INT is > wider than BOOL, but why BOOL is wider than INT64? It isn't actually, but INT64 is a bit of a special case and I had to put it somewhere. > > As reference I have been checking > > https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#property-values [..] Regards, Simon
Hi Simon, On 11/24/21 21:12, Simon Glass wrote: > Hi Walter, > > On Mon, 2 Aug 2021 at 13:29, Walter Lozano <wlozano@collabora.com> wrote: >> Hi Simon, >> >> On 8/1/21 11:50 PM, Simon Glass wrote: >>> Hi Walter, >>> >>> On Sun, 1 Aug 2021 at 20:45, Walter Lozano <wlozano@collabora.com> wrote: >>>> Hi Simon, >>>> >>>> Thanks for checking this bug, I'm glad that you were able to come with >>>> fix quickly. I have some questions, I hope that you find some time to >>>> help me understand. >>>> >>>> On 7/28/21 10:23 PM, Simon Glass wrote: >>>>> The current name is confusing because the logic is actually backwards from >>>>> what you might expect. Rename it to needs_widening() and update the >>>>> comments. >>>>> >>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>> --- >>>>> >>>>> tools/dtoc/fdt.py | 15 +++++++++------ >>>>> 1 file changed, 9 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py >>>>> index 3996971e39c..9749966d5fb 100644 >>>>> --- a/tools/dtoc/fdt.py >>>>> +++ b/tools/dtoc/fdt.py >>>>> @@ -24,16 +24,19 @@ from patman import tools >>>>> >>>>> # A list of types we support >>>>> class Type(IntEnum): >>>>> + # Types in order from widest to narrowest >>>>> (BYTE, INT, STRING, BOOL, INT64) = range(5) >>>> Sorry but I don't understand why BYTE is wider than INT (or INT64) >>> I think perhaps we need a better name. A wider type is one that can >>> hold the values of a narrower one, plus more. >>> >>> In this case a 'bytes' type can hold anything (bytes, int, int64, >>> bool) so is the 'widest' there is. It is the lowest common denominator >>> in the devicetree. >> Thanks for taking the time to explain. I understand the idea behind your >> explanation but I still not sure if I follow you completely. In any >> case, let me add a few words in order to be more clear. >> >> It is my impression that when you say 'bytes' (and not BYTE like in the >> declaration) you are referring to a list. Is that the case? >> >> If not, BYTE (8 bit) seems to be narrower than INT (32 bits), isn't it? >> > A bit long in finding this email again... > > Actually bytes means the Python bytes type which holds a sequence of > bytes. So it can hold an int, whereas an int cannot hold a bytes > value, in general (although it could if it happened to be 4 bytes). It is more clear now, thank you. > >> Also, another example is INT, BOOL and INT64. It is clear that INT is >> wider than BOOL, but why BOOL is wider than INT64? > It isn't actually, but INT64 is a bit of a special case and I had to > put it somewhere. I see, thank you for the explanation! > > As reference I have been checking > > https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#property-values > [..] Regards, Walter
diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index 3996971e39c..9749966d5fb 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -24,16 +24,19 @@ from patman import tools # A list of types we support class Type(IntEnum): + # Types in order from widest to narrowest (BYTE, INT, STRING, BOOL, INT64) = range(5) - def is_wider_than(self, other): - """Check if another type is 'wider' than this one + def needs_widening(self, other): + """Check if this type needs widening to hold a value from another type - A wider type is one that holds more information than an earlier one, - similar to the concept of type-widening in C. + A wider type is one that can hold a wider array of information than + another one, or is less restrictive, so it can hold the information of + another type as well as its own. This is similar to the concept of + type-widening in C. This uses a simple arithmetic comparison, since type values are in order - from narrowest (BYTE) to widest (INT64). + from widest (BYTE) to narrowest (INT64). Args: other: Other type to compare against @@ -149,7 +152,7 @@ class Prop: update the current property to be like the second, since it is less specific. """ - if self.type.is_wider_than(newprop.type): + if self.type.needs_widening(newprop.type): if self.type == Type.INT and newprop.type == Type.BYTE: if type(self.value) == list: new_value = []
The current name is confusing because the logic is actually backwards from what you might expect. Rename it to needs_widening() and update the comments. Signed-off-by: Simon Glass <sjg@chromium.org> --- tools/dtoc/fdt.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)