Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-9273

The xpa wrapper in display_ds9 could use some cleanup

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: To Do
    • Resolution: Unresolved
    • Fix Version/s: None
    • Component/s: display_ds9
    • Labels:
      None

      Description

      The python wrapper for xpa in display_ds9 defines several C/C++ functions that call into xpa and then wraps them for Python. Unfortunately the code is rather messy. Issues I would like cleaned up:

      The functions have no documentation.

      Some of the char * arguments and return values should probably be std::string const & or similar. In particular XPASet1 takes char * buf, int len where len is the number of characters in the buffer. That argument would be much safer as a std::string. One subtlety is that the xpa functions this code calls don't specify const for some of the arguments that clearly are intended to be const.

      The functions are defined with one name, then used in Python with another name. I would be easier to understand the code if the C/C++ names matched the Python names.

      The new functions have no header file and are defined in the same file as the pybind11 wrapper (emulating SWIG). It would be cleaner and more standard to separate the new functions into header and implementation files, with the wrapper separate.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Paging Xiuqin Wu [X] for reference — it's not clear to me if SUIT intends to do any maintenance on the ds9 codebase.

            Show
            swinbank John Swinbank added a comment - Paging Xiuqin Wu [X] for reference — it's not clear to me if SUIT intends to do any maintenance on the ds9 codebase.
            Hide
            xiuqin Xiuqin Wu [X] (Inactive) added a comment -

            John Swinbank This topic never came up in past discussions. We could get the users and developers together and discuss the need and workload. Do you think it could be a topic for JTM?

            Show
            xiuqin Xiuqin Wu [X] (Inactive) added a comment - John Swinbank This topic never came up in past discussions. We could get the users and developers together and discuss the need and workload. Do you think it could be a topic for JTM?
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Other major issues not in the original post:

            • The singleton implementation in myXPA depends on side effects, making it difficult to verify. It also creates multiple XPA objects at the same time, although I'm not clear on whether that was intended.
            • XPASet1 returns a pointer to a local variable on one execution path.
            Show
            krzys Krzysztof Findeisen added a comment - - edited Other major issues not in the original post: The singleton implementation in myXPA depends on side effects, making it difficult to verify. It also creates multiple XPA objects at the same time, although I'm not clear on whether that was intended. XPASet1 returns a pointer to a local variable on one execution path.
            Hide
            swinbank John Swinbank added a comment -

            Reviewed for DM-CCB, 2020-09-16. Still seems like a valid issue, but low priority; I can't imagine us working on this unless it causes problems in practice.

            Show
            swinbank John Swinbank added a comment - Reviewed for DM-CCB, 2020-09-16. Still seems like a valid issue, but low priority; I can't imagine us working on this unless it causes problems in practice.

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              rowen Russell Owen
              Watchers:
              John Swinbank, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:

                  Jenkins

                  No builds found.