1 |
On 23:58 Mon 12 Sep , Michał Górny wrote: |
2 |
> On Mon, 12 Sep 2011 16:00:20 -0500 |
3 |
> Donnie Berkholz <dberkholz@g.o> wrote: |
4 |
> > > local f |
5 |
> > > for f in $(find "${D}" -type f -name '*.la'); do |
6 |
> > > # Keep only .la files with shouldnotlink=yes - |
7 |
> > > likely plugins local shouldnotlink=$(sed -ne |
8 |
> > > '/^shouldnotlink=yes$/p' "${f}") if [[ "$1" == 'all' || -z |
9 |
> > > ${shouldnotlink} ]]; then |
10 |
> > > + if [[ "$1" == 'only-not-required' ]]; then |
11 |
> > |
12 |
> > Is there a case where one of those arguments might be $2 but you'd |
13 |
> > still want to run this? |
14 |
> |
15 |
> Er? What are you referring to? |
16 |
|
17 |
Two things. |
18 |
|
19 |
1. This is only reached if shouldnotlink is false. That means it's only |
20 |
the things that you are already assuming are plugins, right? If so, why |
21 |
is this even done? |
22 |
|
23 |
2. What happens if I call it with `remove_libtool_files all |
24 |
only-not-required`? Nobody ever does any checking of the # of args. |
25 |
|
26 |
> > I feel like that shouldnotlink thing is really confusing the logic, |
27 |
> > because there's multiple nested tests for different values of $1 in |
28 |
> > here instead of just testing the args once at the top and setting |
29 |
> > variables. |
30 |
> |
31 |
> As mentioned earlier, the code needs to be refactored. First things |
32 |
> first, then we'll rewrite it to be nice and clean. I don't really want |
33 |
> to waste time doing this if we would need to rewrite it for more logic |
34 |
> in the future. |
35 |
|
36 |
I'd rewrite it once as soon as you get all the reviews and before |
37 |
committing. Rewrites tend to never happen, since it turns out that |
38 |
people have other things to do with their time. |
39 |
|
40 |
> > > + # remove .la files only when .pc |
41 |
> > > files provide the libs |
42 |
> > > + # already or they don't give any |
43 |
> > > information |
44 |
> > > + ! has $(basename "${f}") |
45 |
> > > ${pc_libs} \ |
46 |
> > > + && [[ -n "$(sed -n |
47 |
> > > \ |
48 |
> > |
49 |
> > The comment says "or" but I see an "and" here. |
50 |
> |
51 |
> Because everything's negated here. Boolean magic :D. |
52 |
|
53 |
OK, got it. Stop writing confusing logic. =P |
54 |
|
55 |
-- |
56 |
Thanks, |
57 |
Donnie |
58 |
|
59 |
Donnie Berkholz |
60 |
Council Member / Sr. Developer |
61 |
Gentoo Linux |
62 |
Blog: http://dberkholz.com |