[Refactor] Deprecate Expr::operator=

[junaire]

Signed-off-by: Jun jun@junz.org

Related issue = #2684

[netlify]

✔️ Deploy Preview for jovial-fermat-aa59dc ready!

🔨 Explore the source changes: 4df5936

🔍 Inspect the deploy log: https://app.netlify.com/sites/jovial-fermat-aa59dc/deploys/61a19831c9c62d000768c812

😎 Browse the preview: https://deploy-preview-3596--jovial-fermat-aa59dc.netlify.app

[junaire]

/format

[strongoier]

/format

[strongoier]

/format

[k-ye]

Hmm, a few test failed. I guess this PR doesn't really cover all the usage of operator=

[junaire]

Hmm, a few test failed. I guess this PR doesn't really cover all the usage of operator=

It is very interesting... Because I found all usages by following go to reference, maybe there are any indirect calls? I wonder code like this whether break or not:

ExprGroup(const Expr &a, const ExprGroup &b) {
    exprs = b.exprs;
    exprs.insert(exprs.begin(), a);
  }

vector::insert calls Expr::operator= internally.

[k-ye]

Hmm, a few test failed. I guess this PR doesn't really cover all the usage of operator=

It is very interesting... Because I found all usages by following go to reference, maybe there are any indirect calls? I wonder code like this whether break or not:

ExprGroup(const Expr &a, const ExprGroup &b) {
    exprs = b.exprs;
    exprs.insert(exprs.begin(), a);
  }

vector::insert calls Expr::operator= internally.

Ah, this could very likely be the reason (https://stackoverflow.com/a/3272731/12003165 says that "as of C++11, standard containers all use copy/move constructors for non-assignment operations (like push_back)", but doesn't say how vector's operator= gets implemented). Pre c++11 does require the value type to be copy-assignable/constructible (https://en.cppreference.com/w/cpp/container/vector)... It shouldn't be too difficult to construct a simple test case that tells
whether T::operator= is used. If so, could you try another way to implement exprs = b.exprs?

[junaire]

Hmm, a few test failed. I guess this PR doesn't really cover all the usage of operator=

It is very interesting... Because I found all usages by following go to reference, maybe there are any indirect calls? I wonder code like this whether break or not:

ExprGroup(const Expr &a, const ExprGroup &b) {
    exprs = b.exprs;
    exprs.insert(exprs.begin(), a);
  }

vector::insert calls Expr::operator= internally.

Ah, this could very likely be the reason (https://stackoverflow.com/a/3272731/12003165 says that "as of C++11, standard containers all use copy/move constructors for non-assignment operations (like push_back)", but doesn't say how vector's operator= gets implemented). Pre c++11 does require the value type to be copy-assignable/constructible (https://en.cppreference.com/w/cpp/container/vector)... It shouldn't be too difficult to construct a simple test case that tells whether T::operator= is used. If so, could you try another way to implement exprs = b.exprs?

Thanks very much for your comment! I'm willing to dig into this issue and try my best to solve it, but it may take some time as I find some build and test problems( Don't worry, I'll figure it out.

BTW, can this annoying issue be used as another argument that we should switch to Rust? (Hahaha, just kidding 🤣

[k-ye]

I'd love to be able to use Rust :-) It's just that the tech stack (mainly the GPU stuff) Taichi depends on is not that mature

[junaire]

I'd love to be able to use Rust :-) It's just that the tech stack (mainly the GPU stuff) Taichi depends on is not that mature

Don't be too serious about this joke 🤣 I know nothing about rust, of course C++ is the same

[junaire]

Emmm... I try to mark operator= as =delete to see where it was called, and got tons of errors. It was like:

...

/home/jun/dev/cpp-projects/taichi/taichi/math/svd.h:115:16: error: overload resolution selected deleted operator '='
  Stiny_number = Tf(1.e-20f);

/home/jun/dev/cpp-projects/taichi/taichi/math/svd.h:118:8: error: overload resolution selected deleted operator '='
  Sa21 = load_if_ptr(a10);
...

That's little scary... I think it may be too hard to completely disable Expr::operator= for now. In my opinion, maybe we can offer two methods set_or_assign and operator=, like what we did in 5392e9e, so the existing code wouldn't broke.

What's your idea? @k-ye

[yuanming-hu]

Sorry about the mess in svd.h - that file was translated from a piece of SEE/AVX vectorized code to the legacy Taichi C++ frontend. I wrote a Python script for the translation two years ago. We may need to use regex to conduct batch replacement in that file. The replacement may need to go into a separate PR.

In the long run, since svd.h contains too many legacy C++ frontend constructs, maybe we should consider using the CHI IR builder to replace the machine-generated code, after we have Taichi function support. Note that the SVD function there emits a frontend AST and needs lower_ast(...). WDYT? @k-ye (And maybe we need a standard library for functions like this...)

[k-ye]

My bad. Didn't realize that this was a lot harder than expected. Thanks for pinning the problems!

In my opinion, maybe we can offer two methods set_or_assign and operator=, like what we did in 5392e9e, so the existing code wouldn't broke.

Sounds great!

bug

[k-ye]

Sorry about the delay, LGTM!