Problems in mpint_field.cc and item.h

[maths644311798]

(1)
In mpint_field.cc,

void MPIntField::NegInplace(MPInt *x) const {
  if (x->IsZero()) {
    return;
  }

  WEAK_ENFORCE(IsInField(*x), "x is not a valid field element, x={}", *x);
  x->NegateInplace();
  AddInplace(x, mod_);
  x->DecrOne();
}

This function does not need x->DecrOne();.

(2)
In item.h,

template <typename T>
  absl::Span<const T> AsSpan() const {
…
static_assert(!std::is_same_v<bool, RawT>,
                    "Call AsSpan<bool> on a vector item is not allowed");
…

Why "Call AsSpan<bool> on a vector item is not allowed"?

[usafchn]

(1)
You are right! I think it is a mistake.
Since there are no unit tests for all xxxInplace functions currently, this part of the code may also contain other errors. We will complete the unit tests as soon as possible.

(2)
Because std::vector is not a real vector type. It is a proxy type. It's somewhat similar to a bitset so that you can't get the address of each bit.
See:
https://stackoverflow.com/questions/17794569/why-isnt-vectorbool-a-stl-container
abseil/abseil-cpp#644
https://en.cppreference.com/w/cpp/container/vector_bool

[maths644311798]

@usafchn ,
(3) I also doubt that in arg_kv.h,

  template <typename T>
  SpiArg operator=(const T &value) {
    value_ = value;
    return *this;
  }

Should it be &SpiArg operator=(const T &value) {?
(4) In mpint_field.h, can void NegInplace(MPInt *x) const override; cover class GaloisField's virtual void NegInplace(Item* x) const = 0;? The main problem is the conversion between MPInt* and Item* .

[usafchn]

@usafchn , (3) I also doubt that in arg_kv.h,

  template <typename T>
  SpiArg operator=(const T &value) {
    value_ = value;
    return *this;
  }

Should it be &SpiArg operator=(const T &value) {? (4) In mpint_field.h, can void NegInplace(MPInt *x) const override; cover class GaloisField's virtual void NegInplace(Item* x) const = 0;? The main problem is the conversion between MPInt* and Item* .

首先，本 pr 前面提到的一些问题连同 mpint 其它的一些 bugfix 将在下一次 sync 的时候同步 github，我就不直接在 github 上改了。

(3) 是一个很好的建议，我改成返回引用
(4) 应该不涉及类型强转。这里的继承关系是 MPIntField -> GFScalarSketch -> GaloisField， GFScalarSketch<MPInt> 会把所有 Item 转换成实际类型，本质上，GFScalarSketch<MPInt> 实现了 GaloisField 中所有的虚函数 同时自己又定义了一组同名的新的虚函数，只是参数不一样，MPIntField 实现的是 GFScalarSketch<MPInt> 定义的虚函数，没有直接实现 GaloisField 中的函数。

关于第三代 SPI 架构设计，目前还没有公开，后面我整理一下放到官网文档中，大意就是：

[maths644311798]

对于GaloisField的SPI，类似GaloisFieldFactory::Instance().Create(kFieldName, ArgLib = kLibName,ArgMod = 13_mp)得到的unique_ptr<GaloisField> p将很难调用MPIntField::NegInplace(MPInt *x)，因为中间隔了一层GFScalarSketch. 似乎要对指针做类型转换才能调用MPIntField::NegInplace(MPInt *x).

[usafchn]

对于GaloisField的SPI，类似GaloisFieldFactory::Instance().Create(kFieldName, ArgLib = kLibName,ArgMod = 13_mp)得到的unique_ptr<GaloisField> p将很难调用MPIntField::NegInplace(MPInt *x)，因为中间隔了一层GFScalarSketch. 似乎要对指针做类型转换才能调用MPIntField::NegInplace(MPInt *x).

对于 p->NegInplace(Item *) 这个例子，Item 表示 GaloisField 中的一个 Element，不一定就是 MPInt。至于 Item 的实际类型，是 Lib 自己定义的类型，可以是任意类型，对用户完全是黑盒。

虽然对于 MPInt Field, 其类型已知是 MPInt，但是对于别的 GaloisField，比如扩域，二进制域（Binary Field），甚至多项式域，其类型根本不是 MPInt，域底层的集合也不一定定义在整数 $\mathbb{Z}$ 上，这里的 Element's Type 完全就是个黑盒，未来，如果 Lib 基于硬件加速实现的，底层数据甚至有可能不在内存，这里的变量只保存指向显存的句柄，因此用户程序不应该与实际类型绑定，否则失去了平滑在不同 Lib 之间切换的能力，正确的方式应该是：

auto gf = GaloisFieldFactory::Instance().Create(kFieldName, ArgMod = 13_mp);
// use gf
// gf->XXXX() 

GaloisFieldFactory 将自动选定最合适的 Lib。

至于用户怎么创建 Item，需要一个从“共识”类型 Map 到 Field Element，即 Encoders 或 Converters，Encoder/Converter 接口会随着 Lib 的增加逐步完善。

另外 GaloisField 中出现了 MPInt，比如 virtual MPInt GetOrder() const = 0，这是因为 MPInt 本身是 YACL 的基础设施之一，是一个“共识”类型，无论哪种 GaloisField 这个接口一定返回整数类型。

[maths644311798]

(^_^)，我不知道下一次Repo Sync会修改哪些内容。我先在PR里面改了mpint_field和arg_kv等文件。

[usafchn]

(^_^)，我不知道下一次Repo Sync会修改哪些内容。我先在PR里面改了mpint_field和arg_kv等文件。

好的，已经合入，感谢！~