pombreda / ctypesgen

Automatically exported from code.google.com/p/ctypesgen
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

find_names_in_modules code incorrect in r121 #22

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
names.union(dir(module)) will not actually alter names.

This function will always return the empty set.

Original issue reported on code.google.com by pmaupin on 14 Sep 2011 at 9:06

GoogleCodeExporter commented 9 years ago
Did you really mean r121? I.e. 
http://code.google.com/p/ctypesgen/source/detail?r=121 I can't see a change 
like "names.union(dir(module))" in r121.

If you are able to start a code review on the change and line(s) that are 
problematic, that may show the problem more clearly.

Original comment by clac...@gmail.com on 14 Sep 2011 at 11:02

GoogleCodeExporter commented 9 years ago
Sorry, didn't mean changed in rev 121.  Meant that I was browsing the trunk, 
and I gave the version of the trunk that I was looking at, in case somebody 
noticed and fixed it after that time.

Here's the sequence of events:

1) I was looking for something like this; found the project.

2) Was looking for doc; found a little, then decided to look at the source to 
find a bit more info.

3) Was looking at the source; found by inspection that the very first function 
(find_names_in_modules) in the very top module (ctypesgen.py) will never return 
anything but an empty set, for the simple reason that line 13 reads:

            names.union(dir(module))

when it obviously was meant to be:

            names.update(dir(module))

(because "union" returns a *new* set consisting of the union of the set and an 
iterable, while "update" updates the set by adding each element of the iterable 
to the *original* set.)

4) Made a too-terse bug report -- sorry about that -- have been distracted 
lately.

That is all.

Thanks and best regards,
Pat

Original comment by pmaupin on 15 Sep 2011 at 12:00

GoogleCodeExporter commented 9 years ago
You've definitely found something odd so thank you for that!

I've not had to deal with this code before (i.e. it hasn't failed for me yet), 
as names is only declared here an update does seem more appropriate :-) I've 
CC'd the original change author of r47 (from svn annotate) TimMaxw.

There is a bare except which is something we should try and avoid too.

Have you had code that caused a problem here? The reason I ask is that if you 
do, maybe we can add that as a regression test to the suite. It is always nice 
to have proof that a code change had a (positive) impact.

Thanks again for the report.

Original comment by clac...@gmail.com on 15 Sep 2011 at 12:25

GoogleCodeExporter commented 9 years ago
"Have you had code that caused a problem here?"

Haven't even tried to run it yet -- as I said I was just browsing to try to 
figure out the best way to use it.  I tend to over-analyze things and spend a 
lot of time researching before I actually *do* anything.

I have a set of .h files along with a set of .so libraries.  They are really 
part of one system, so I'd like a single module to interact with them.  I 
haven't tried it yet, though.

Original comment by pmaupin on 15 Sep 2011 at 12:39