Opened 3 years ago

Closed 3 years ago

#24607 closed enhancement (fixed)

Add abstract _add_ and _mul_ methods to RingElement

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

Status badges

Description (last modified by jdemeyer)

In order to fix all Compatible but non-identical C method '_add_' not redeclared in definition part of extension type '...'. This may cause incorrect vtables to be generated. warnings, we add abstract methods _add_ and _mul_ to some base Element classes.

This is part of #23600.

In #24116, such abstract methods were added to the finite ring element base class. Those can be removed again.

Change History (14)

comment:1 Changed 3 years ago by jdemeyer

  • Cc tscrim added
  • Description modified (diff)

comment:2 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/add_abstract__add__and__mul__methods_to_ringelement

comment:4 Changed 3 years ago by jdemeyer

  • Commit set to 8180e22ae94307e70927d198edf7c351397a7d73
  • Status changed from new to needs_review

New commits:

​8180e22Add abstract _add_ and _mul_ methods

comment:5 Changed 3 years ago by tscrim

I know it doesn't really matter, but the error string for the NotImplementedError I think is not formatted properly (missing the leading f).

comment:6 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

Otherwise LGTM and you can set a positive review on my behalf either way.

comment:7 Changed 3 years ago by jdemeyer

Ooops...

comment:8 Changed 3 years ago by git

  • Commit changed from 8180e22ae94307e70927d198edf7c351397a7d73 to 4715809a8aaf336a92775902e747a69bb1505a67

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

​4715809Add abstract _add_ and _mul_ methods

comment:9 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Thanks.

comment:10 Changed 3 years ago by jdemeyer

  • Status changed from positive_review to needs_work

There are some unexpected doctest failures.

comment:11 Changed 3 years ago by git

  • Commit changed from 4715809a8aaf336a92775902e747a69bb1505a67 to d984df507be6b352f87bafbc388e23d7e5995031

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

​d984df5Fix GapElement_generic._add_

comment:12 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:13 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Tests now pass.

comment:14 Changed 3 years ago by vbraun

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