Opened 4 years ago

Closed 4 years ago

#23943 closed enhancement (fixed)

C++ clean up in polybori interface

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.1
Component: cython Keywords:
Cc: tscrim, fbissey Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: feee399 (Commits, GitHub, GitLab) Commit: feee3997aa8f87f2eaf900c12e0a87cd27ce4dbb
Dependencies: #23857, #21083 Stopgaps:

Status badges

Description (last modified by jdemeyer)

Use the C++ features of Cython instead of using ccobject.h hacks.

The interface is significantly more complicated than some other C++ interfaces. There are also some Cython oddities that I have not investigated further. Therefore, the result may not be super clean, but it's certainly a large improvement over the current situation.

Change History (17)

comment:1 Changed 4 years ago by jdemeyer

  • Cc tscrim added
  • Description modified (diff)
  • Summary changed from C++ clean up in polybori to C++ clean up in polybori interface

comment:2 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/23943

comment:3 Changed 4 years ago by git

  • Commit set to 9ec349e6ad2516e7b326ac2212926993c44e7a82

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9ec349eC++ clean up in polybori

comment:4 Changed 4 years ago by jdemeyer

This is harder than some other library clean up ticket. In particular I do not understand why some things are wrapped as DefaultRinged<T> instead of just T.

For example, why

cdef cppclass PBSet "DefaultRinged<BooleSet> "

comment:5 Changed 4 years ago by git

  • Commit changed from 9ec349e6ad2516e7b326ac2212926993c44e7a82 to 2314a54346a6978319a126e4a9f161361dd9ff33

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2314a54C++ clean up in polybori

comment:6 Changed 4 years ago by jdemeyer

  • Dependencies changed from #23857 to #23857, #21083

New commits:

2314a54C++ clean up in polybori

comment:7 Changed 4 years ago by git

  • Commit changed from 2314a54346a6978319a126e4a9f161361dd9ff33 to dcefde532346d2ea29e3268c9985bb97aaba81d7

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

82e4fcaUpgrade BRiAl to version 1.0.1
89f4ca7Merge commit 'b7127ce548910ff52d71867a817ab9485ca5636b' into HEAD
dcefde5C++ clean up in polybori

comment:8 Changed 4 years ago by git

  • Commit changed from dcefde532346d2ea29e3268c9985bb97aaba81d7 to d4f61b8a660b1002fca65884ee05aca4d818c518

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d4f61b8C++ clean up in polybori

comment:9 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from new to needs_review

comment:10 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 4 years ago by git

  • Commit changed from d4f61b8a660b1002fca65884ee05aca4d818c518 to bfeac9b4f08d17afa0081661e0045440a1411bd0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bfeac9bC++ clean up in polybori

comment:12 Changed 4 years ago by fbissey

  • Cc fbissey added

comment:13 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

The reason I can gather for the wrapping is a template approach to giving a default value for the ring, but I haven't looked to deeply into it.

However, this is definitely an improvement, so positive review.

comment:14 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:15 Changed 4 years ago by git

  • Commit changed from bfeac9b4f08d17afa0081661e0045440a1411bd0 to feee3997aa8f87f2eaf900c12e0a87cd27ce4dbb

Branch pushed to git repo; I updated commit sha1. New commits:

feee399Merge tag '8.1.beta8' into t/23943/ticket/23943

comment:16 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to positive_review

Merge conflict fixed.

comment:17 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/23943 to feee3997aa8f87f2eaf900c12e0a87cd27ce4dbb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.