On Sat, 18 Apr 2009, Modestas Vainius wrote: > > wildcard=c++:"Foo::Bar::Private::foobar()"@Base 1.0 > > optional:wildcard=c++:"Foo::Bar::Private::foobar()"@Base 1.0 > Hmm, I fail to understand what this = means here.
?? It's just a separator between the tag and its value. > From a93c9c0eaf45662f89997ec60a01ac635a3f10fe Mon Sep 17 00:00:00 2001 > From: Modestas Vainius <[email protected]> > Date: Sat, 18 Apr 2009 22:13:33 +0300 > Subject: [PATCH] Support for symbol tags and implementation of optional/arch > tags. > > Symbols might be tagged with arbitary number of tags which are > separated by '|' (aka pipe) character. Tags (only if there are any) must be > enclosed in the () brackets right before symbol name. Tags can have arbitary > number of arguments (parameters). Arguments follow the tag name after the '=' > character and are separated by commas (','). Arguments might be quoted with > either " or ' characters if they contain special characters like " ' | = , ( > ). If the symbol specification has tags, the symbol name itself can be quoted > too if it contains whitespaces (which are normally not allowed). If there are > no tags, symbol name cannot be quoted (like before). > > Example of a quoted symbol with 3 tags: > > (tag1="\"param1, val",,"param2"|tag2=param1|tag3)"Foo::Bar::foobar()"@Base > 1.0 1 I don't want unneeded complexity. We don't need to support arbitrarily complex values in tags I think. So we should forget about quoting special chars in tags and stuff like that. Ie forbid "=()|" in tags and their values. Same goes for the symbol name, it can be quoted but then no need to allow quotes (") in their name. The comma should not be special, it can be used as delimiter for multiple values in a tag but no special support for that should be put in place. > As a result of tag support, a new parameter 'template' has been added to > SymbolFile::dump(). If template is not set (default), dump() strips tags when > dumping symbols . If template is set, dump() keeps tags in the output. Fine. > * arch=<arch name or alias>,... AND !arch=<arch name or alias>,... - allows to > mark a symbol as arch-specific. When dumping in non-templace mode, only Please use a single tag name for this. It should be "arch=!arm" and not "!arch=arm". We should really use/allow the same syntax than in Build-Depends. > +use Text::Balanced qw( extract_delimited extract_bracketed extract_multiple > ); I hope we can avoid relying on this due to the simplified rules of what's allowed. > +sub new { > + my ($cls, %args) = @_; > + my $self = bless { > + symbol => undef, > + minver => undef, > + dep_id => 0, > + deprecated => 0, > + tags => [], > + }, $cls; Please use the same code for new() like in other objects where you can $obj->new() as well as My::Class->new() Why use an array for the tags and not directly an hash ? (The order in the output is not important, it could be alphabetically sorted) It would simplify quite some code later. > + map { $self->{$_} = $args{$_} } keys %args; $self->{$_} = $args{$_} foreach keys %args; (using map for a loop is not nice) > + return $self; > +} > + > +sub parse { > + my ($self, $symbolspec) = @_; > + my $symbol; > + my $rest; > + > + if ($symbolspec =~ /^\(/) { This function should be simplified based on the comments above. > +sub get_name { > + my $self = shift; > + my $symbol = $self->{symbol}; > + $symbol =~ s/['"']//g; > + return $symbol; > +} The quotes should be stripped in parse() and not be stored ? (And then restored in dump() only if necessary) > +sub get_tag_args { > + my $self = shift; > + my $tag = shift; > + my @args; > + for (@{$tag->{args}}) { > + s/^["'](.*)["']$/$1/; > + s/\\(["'])/$1/g; > + push @args, $_; > + } > + return @args; > +} Ouch... so much code when it should be a simple hash lookup. :-) > @@ -73,8 +281,9 @@ unwind_cpp_pr2 uread4 uread8 uwrite4 uwrite8)); > sub new { > my $this = shift; > my $file = shift; > + my %op...@_; If you add supplementary key-based arguments, you should convert the current single argument to also be key based (key "file" for example). > my $class = ref($this) || $this; > - my $self = { }; > + my $self = \%opts; > bless $self, $class; > $self->clear(); > if (defined($file) ) { > @@ -117,38 +326,33 @@ sub load { > my $obj; > $current_object_ref = \$obj; > } > - local *object = $current_object_ref; > + my $object = $current_object_ref; Hum, not sure this is right. I don't remember the details but it was tricky to make it work. The goal was to pass along the value despite #include leading to recursive calls. A change to $object had to modify the other variable so that when we recurse out it keeps the new object created by one of the sub-calls. > + } > } > - # We assume that the right dependency information is already > - # there. > - if (vercmp($minver, $info->{minver}) < 0) { > - $info->{minver} = $minver; > + if (!$info->is_from_arch($self->{arch})) { > + # Remove arch tags because they are incorrect. > + $info->delete_tags("arch", "!arch"); A warning is in order I think. > # Get the info from wildcards > $info = $obj->{wildcards}{$symobj->{version}}; > $self->{used_wildcards}++; > } else { > # New symbol provided by the current release > - $info = { > - minver => $minver, > - deprecated => 0, > - dep_id => 0 > - }; > + $info = new Dpkg::Shlibs::SymbolFile::Symbol(symbol => $sym, > minver => $minver); We use Class->new() everywhere else in the code. Once those changes/cleanup are done, we are not far from something that can be committed. We still need documentation update of the dpkg-gensymbols manual page for the new features. We probably want to add an option to make dpkg-gensymbols dump out the tags too. Cheers, -- Raphaël Hertzog Contribuez à Debian et gagnez un cahier de l'admin Debian Lenny : http://www.ouaza.com/wp/2009/03/02/contribuer-a-debian-gagner-un-livre/ -- To UNSUBSCRIBE, email to [email protected] with a subject of "unsubscribe". Trouble? Contact [email protected]
