Hello, On 2009 m. May 14 d., Thursday 23:47:44 you wrote: > (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.
> > +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.
I don't see a big problem with that class though. Pretty smart parser is fast
to implement with Text::Balanced, but may take a bit more processing time.
However, do not forget that tags are supposed to be rare so impact on
performance should be minimal.
,, syntax was there not to choke on empty tag values.
However, ok, I'll implement this with plain regexps forbidding special
characters when needed. Generally, if I forbid m/[(),|=]/ in tag names and
values, I don't need to support quoting for tag values anymore, as I don't win
anything from them.
> > * 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.
Probably I can do that and I'll have a look at build depends parser.
>
> > +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()
ok
>
> 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.
> > +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)
No, it is not that simple. dump() is supposed to dump the same symbol
specification as has read it not to trigger useless dpkg-gensymbols diffs.
Which means that original tag order, quoted/not quoted status must be known.
As tag value quoting is going away, I could probably set $self-
>{symbol_quoted} to indicate wheather symbol name should be quoted on dump()
or not.
I'm waiting for your opinion on this one before fixing the patch.
>
> > +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. :-)
I didn't want to duplicate tag collection in the hash map as I need tag array
anyway (see above). This was also created for another reason: to keep info
about quoting status of the tag values (for dumping) which becomes irrelevant
if quoting of tag values is no longer needed.
>
> > @@ -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).
I somewhat wanted to keep API backwards compatible.
>
> > 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.
IIRC, that "local" stuff errored out on me. That's the only reason I changed
it. I'll see what went wrong there.
>
> > + }
> > }
> > - # 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.
Removal of the arch tag makes the symbol appear in the dpkg-gensymbols diff.
That's the point of all this. A simple warning might get lost in the build
log.
>
> > # 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.
ok
> 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.
I'm very slow at writing docs.
> We probably want
> to add an option to make dpkg-gensymbols dump out the tags too.
I'm not sure what you mean here. dpkg-gensymbols dumps tags when diff'ing two
symbol file templates. Dumping tags to DEBIAN/symbols does not make sense as
syntax is not backwards compatible with previous versions.
--
Modestas Vainius <[email protected]>
signature.asc
Description: This is a digitally signed message part.
