Re: [PATCH 4/4] Create a new property value that means 'undefined'.

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

On Wed, Oct 20, 2010 at 11:20:53PM -0600, Grant Likely wrote:
> On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:
> > When updating existing nodes in a device tree merge operation, properties
> > can be removed by setting the value to /undef-prop/.
> > 
> > if /undef-prop/ is assigned to a property that doesn't exist, the property
> > is treated the same as if it had not been declared.
> > 
> > Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> 
> Implementation looks good.
> > ---
> > 
> >  dtc-lexer.l  |   14 ++++++++++----
> >  dtc-parser.y |    6 ++++++
> >  dtc.h        |    7 +++++++
> >  flattree.c   |    3 +++
> >  livetree.c   |   38 +++++++++++++++++++++++++++++++++-----
> >  5 files changed, 59 insertions(+), 9 deletions(-)
> > 
> > diff --git a/dtc-lexer.l b/dtc-lexer.l
> > index 216a3d2..efa89b4 100644
> > --- a/dtc-lexer.l
> > +++ b/dtc-lexer.l
> > @@ -102,6 +102,12 @@ static int pop_input_file(void);
> >  			return DT_REMOVENODE;
> >  		}
> >  
> > +<*>"/undef-prop/"	{
> > +			DPRINT("Keyword: /undef-prop/\n");
> > +			BEGIN_DEFAULT();
> > +			return DT_UNDEFINED;
> > +		}
> > +
> 
> Does /undef-prop/ really need to be using <*> to match in all start
> conditions?

It doesn't need to, but it's a good idea for it to do so, because if
the keyword is lexed as a keyword everywhere, it will lead to more
meaningful error messages if it's put somewhere it shouldn't be.

In fact, something I've learnt writing dtc is that in general you
should make your lexical tokens as wide as they can without colliding
with each other, then check that they have the right contents later.
That way you get a clear error message from the checking code
("such-and-so contained an illegal character"), rather than the lexer
breaking it into different tokens instead and the parser generating
some cryptic error.

[snip]
> > @@ -178,6 +179,11 @@ propdef:
> >  		{
> >  			$$ = build_property($1, empty_data);
> >  		}
> > +	| DT_PROPNODENAME '=' DT_UNDEFINED ';'
> 
> Hmmm.  I'm going to make this comment once, but I'll shut-up if you
> guys disagree with me because the details have already been hashed out
> several times, and I've already said I'd be okay with the above form.
> 
> The more I look at it, the more I prefer the form 
> 	/undef-prop/ property;
> instead of
> 	property = /undef-prop/;
> 
> The reason being is that while the assignment form does work, it isn't
> a very natural construct.  Removal is not logically the same as
> assignment.  /undef-prop/ is something that is performed on a
> property.  Syntax that shows /undef-prop/ being assigned as a property
> value doesn't ring true for me as the right thing to do.
> 
> So, my vote is for the "/undef-prop/ property;" form, but I hold my
> piece if you both disagree with me.

I.. yeah.. I'm not sure.  I was leaning towards prop = /undef-prop/;
before, but you've more-or-less persuaded me here.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson