Hi Modestas,
thanks for your work! Here are the results of my review.
On Tue, 05 Jan 2010, Modestas Vainius wrote:
> a7533ad Add Cppfilt module - interface to the c++filt utility.
What happens when the toolchain starts using a (supposedly new) gnu-v4
method of mangling? How will we deal with that?
Also what about support of other OS? How can we plug it in that code when
necessary? (trying to answer myself) I guess we can use the Config module and
select the right type automatically.
> c206e18 Add pattern related subs to Symbol interface.
Small error spotted:
--- a/scripts/Dpkg/Shlibs/Symbol.pm
+++ b/scripts/Dpkg/Shlibs/Symbol.pm
@@ -241,7 +241,7 @@ sub equals {
# Compare names and tag sets
return 0 if $self->{symbol} ne $other->{symbol};
- return 0 if scalar(@{$self->{tagorder}}) != scalar(@{$self->{tagorder}});
+ return 0 if scalar(@{$self->{tagorder}}) != scalar(@{$other->{tagorder}});
for (my $i = 0; $i < scalar(@{$self->{tagorder}}); $i++) {
my $tag = $self->{tagorder}->[$i];
> 76b077b Replace Symbol::clone() with dclone() and sclone().
Why did you decide to have the two variants? Is it only a matter of
saving memory in some cases when the deep clone is not required?
Does it make a significant difference?
> c268d0d Initialize patterns.
+ # Wildcard is an alias based pattern. It gets recognized here even if it is
+ # not specially tagged.
+ if (my $ver = $self->get_wildcard_version()) {
+ error(_g("you can't use wildcards on unversioned symbols: %s"), $_) if
$ver eq "Base
+ $self->init_pattern(($self->is_pattern()) ? 'generic' :
'alias-wildcard');
+ $self->{pattern}{wildcard} = 1;
+ }
Why the test on is_pattern() to initialize it either as generic or
alias-wildcard?
I imagined it could be in case you combine wildcard with regexp pattern but I
fail to see how it could work in practice, the initialization as regex pattern
comes right after that block...
> 9676657 Improve SymbolFile::dump() output.
To me it seems more logical to show pattern matches after the pattern symbol,
no? Is there a reason why you put them first?
> b55f6b3 Document patterns in the manual page.
+counterpart defined in the symbol file. Whenever the first matching pattern is
+found, all its tags and properties will be used as a basis specification of the
+symbol. If neither pattern matches, the symbol will be considered as MISSING.
MISSING -> new. Or did I miss something?
+Please also note that patterns are in the same way affected by \fIoptional\fR,
+\fIarch\fR and other standard tags whenever possible.
How "optional" affects patterns ought to be clarified (see below discussion
about how it should maybe apply by default to wildcards).
+At the moment, \fBdpkg\-gensymbols\fR supports three atomic pattern types:
Why "atomic"?
+for them). However, as these collisions happen on the ABI level, they should
+not degrade quality of the symbol file.
Can you explain more precisely why? (not necessarily in the manual page, but at
least for my own culture)
---
About wildcard symbols, I'm not sure it makes sense to keep the current
syntax only. We should introduce a new syntax:
(match-version)<version>
The old syntax *@<version> would auto-translate to:
(match-version|optional)<version>
That way we keep backwards compatibility with the current behaviour where
an unused wildcard is not a problem. (Not sure optional can be used for this
purpose... otherwise we have to add "optional-pattern" and fix
$sym->mark_not_found_in_library accordingly).
I expect the glibc to have some generice wildcard entries used for all arches
but only some of them are used for a given arch (because support for the arch
has been added at a different point in time) and currently dpkg-gensymbols
would fail complaining that a pattern match got removed/is unused.
If you can think of a better name than "match-version", please suggest
it... but right now I can't.
Otherwise it looks mostly fine. Some stylish comments though:
Please avoid using
my $a = shift;
my $b = shift;
my $c = shift;
Prefer:
my ($a, $b, $c) = @_;
readline($fh) should also be replaced by the more common <$fh>
> It would be great to see this feature in squeeze...
I think it should be ok for squeeze. :)
Cheers,
--
Raphaël Hertzog
--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]
