Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to FlagHandler so that it can be used for Python only plugins #41

Merged
merged 3 commits into from Jun 14, 2016

Conversation

natelust
Copy link
Contributor

Add method to initialize FlagHandler with a vector of FlagDefinitions
Allow FlagDefinition to be constructed from a pair of strings
unit test testFlagHandler added

name = _name;
doc = _doc;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am slightly worried what is going to happen when you have python strings mapped to c style pointers. If the python strings somehow get cleaned up. What will happen to the pointers. Is it practical to use std::strings, or am I being overly worried?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I am not sure about the lifetime issue here, but it certainly should be thought about. Doesn't the same issue come put for the original addFields() call? Seems like what protects those pointers is that they are attached to a static member of the class which is using them. That means that someone abusing the original addFields() could probably create the same issue. But I propose that I adopt the same strategy with Python, and use a static list for the flagDefs and only call the addFields from the plugin constructor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would indeed cause problems if called from Python. But the reason we needed to use raw pointers instead of std::string (direct initialization of an array of struts containing strings) has gone away in C++. Please try just switching to std::string.

    Add method to initialize FlagHandler with a vector of FlagDefinitions
    Allow FlagDefinition to be constructed from a pair of strings
    unit test testFlagHandler added
@pgee2000 pgee2000 force-pushed the tickets/DM-4009 branch 2 times, most recently from 01dc532 to a7ad3ca Compare May 25, 2016 19:53
@@ -39,8 +39,17 @@ namespace lsst { namespace meas { namespace base {
C-strings so we can create these arrays using initializer lists even in C++98.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the comment above to match the changes you've made below.

@pgee2000 pgee2000 force-pushed the tickets/DM-4009 branch 2 times, most recently from c991085 to 363378f Compare June 14, 2016 09:51
Nate's changes to the python only error handler

Provides a decorator class which creates an enumeration for the flagHandler
error numbers, and protects against redundancy of the flag names.
Error constants are named after the flag names, which should be easy to use.
@pgee2000 pgee2000 merged commit 401452b into master Jun 14, 2016
@ktlim ktlim deleted the tickets/DM-4009 branch August 25, 2018 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants