On 09/03/2012 07:29 PM, Gregory Szorc wrote:
I like it! This is much needed.
My initial thoughts were that the Config class is a bit heavyweight. IMO
a module-level function to force reload the global lib instance to point
to a different library would be sufficient. But, thinking it through,
I'm not sure how this could be done without making things weird. The
best I can think of is that at module load time it would attempt to find
a library. If it doesn't, lib is None and the first use of functionality
results in an opaque message about None not having some attribute. If
someone wished to point to a non-default path, they could import the
module and call a function which replaced the module variable "lib."
But, this is essentially what you have coded in the Config class. So, I
guess the extra complexity is warranted. That being said, I'm not
convinced a class is needed (a few module-level functions and variable
would suffice, IMO). But, I don't feel too strongly, so LGTM.
Hi Gregory,
thanks for the review. I was myself thinking if an entire class for this
is worth it. There have been two reasons I decided to go for it:
- It can group further options that may be added in the future
- We can provide better error messages
The second point was probably not clear in my patch. I committed a
slightly modified version, which now points to Config.set_library_path()
in case libclang can not be found. Nothing fancy, but it may be useful.
And more important, we will not have random 'None not having attribute'
messages, but we can decide ourselves which error message to send.
I committed the slightly changed version in 163121.
Tobi
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
