Change *Opers classes to namespaces to flatten hierarchy

Issue #2 resolved
Sam Preston created an issue

Right now IOpers, FOpers, etc. are classes even though they contain only static member functions. If they were changed to namespaces, swig would flatten them so the contained functions could be called directly in python instead of requiring the added 'Oper.' prefix. This may require a little renaming of functions to avoid name collisions. This would also require removal of the prefixing from existing python code unless we hack in fake python modules to create Oper.? aliases, allowing calls both ways.

Comments (8)

  1. Jacob Hinkle

    At this point we don't have that much python code floating around, and if it's as simple as :%s/?Opers.//g then I'd say we don't need to waste time with the fake modules

  2. Sam Preston reporter

    (Reply via js...@sci.utah.edu):

    Yeah, should take advantage of not having to maintain backward compatibility for now

  3. Sam Preston reporter

    I've been looking at this, and it's not going to be as easy as I'd hoped. According to the swig documentation (swig namespace doc), conflicting names in different namespaces generate errors, even when they could be correctly overloaded if they occurred in the same namespace. It's probably still possible, but would require some restucturing since we have lots of conflicting names (Max, Add, etc)

    EDIT: I just checked, and it does work if we dump everything into a single Opers namespace

  4. Jacob Hinkle

    What happens if we just remove the *Opers classes in the C++ code and keep those functions in the same namespace? There are never any conflicting signatures are there?

  5. Sam Preston reporter

    Yeah, I think dumping the whole python math interface into a single Opers namespace is probably the best solution. There will be a few name conflicts, but not many since in most cases it will just overload the functions. Trying it now to see if there are any headaches I haven't thought about.

  6. Sam Preston reporter

    I've just checked in a branch that flattens the namespace (gets rid of all *Oper prefixes), and also makes some other minor naming cleanups (applyU is now applyV to match composeHV, composeVInvH, etc). All testing and examples have been modified to work with the new names. It all works but I didn't want to merge it quite yet since it'll break compatibility of any scripts.

  7. Jacob Hinkle

    Excellent. I personally prefer to get these big changes merged earlier rather than later. If you want to wait so as not to break LayeredDef stuff then that's cool too.

    But for instance Sarang and Nikhil are going to start learning this codebase, so might as well pull the trigger if you ask me.

    Might as well start a pull request and we can move this conversation over there.

  8. Log in to comment