CFA: Simplify the Code

[johannesvollmer]

Some little code size reductions were made, like replacing the manual Clone implementation with a derive

(Outdated: Changed pattern usize to u8, but reverted)

[pedrocr]

Thanks. I'd have to benchmark this as the reason for having usize is that those numbers will often be used as offsets in demosaic implementations so the cast may not be ideal.

Why are you so focused on CFA? It's a very small utility struct in the scheme of things.

[johannesvollmer]

The CFA file was still opened from last time. Also, I really want to see the manual clone implementation replaced by a derive.

Please do some benchmarks. I myself did only a very simple benchmark with bin/benchmark.rs::main and did not record any great differences for the one file that I tested.

[pedrocr]

In fact u8 is almost certainly an issue in some architectures because of memory alignment problems, it should stay usize. The others are mostly cosmetic but please make sure they compile on rust 1.22 as they may depend on newer features.

[pedrocr]

Please do some benchmarks. I myself did only a very simple benchmark with bin/benchmark.rs::main and did not record any great differences for the one file that I tested.

The issue won't show up with that benchmark. CFA is used mostly in client code not in rawloader itself. That client code requires color_at() to be really fast as it will be called inside hot loops. That's why it's a 48x48 array lookup instead of being computed for every call.

[pedrocr]

Also, I really want to see the manual clone implementation replaced by a derive.

Why care so much? It was manual because at some point deriving it didn't work. But it makes no difference for calling code.

[johannesvollmer]

It was manual because at some point deriving it didn't work. But it makes no difference for calling code.

Yes that's perfectly fine, but nevertheless:

Why care so much?

Reducing code size makes maintenance easier. Don't you think so?

[pedrocr]

Reducing code size makes maintenance easier. Don't you think so?

Oh sure, I'll take the change, I was just curious why you seemed so interested in it.

[johannesvollmer]

The issue won't show up with that benchmark.

Oh yeah of course, I just forgot that loading does not demosaic, oops. If you do have some benchmarking setup, feel free to share it. Or did I just not see that you already did?

[pedrocr]

This doesn't actually compile anymore and has too many commits. Maybe just fix the issues and squash into a single commit?

[johannesvollmer]

Oh, yes. How do I tell Rust / Cargo to use an older compiler version? I Googled but I could not find anything

[pedrocr]

Not sure what you mean about older versions. The code that's failing is the one you changed. You now have half the code with u8 and half with usize.

[johannesvollmer]

How embarrassing, I thought I had tested this, sorry, this commit is really anything but compiling

[johannesvollmer]

But nevertheless I need to make sure it compiles with 1.22, but I have only 1.27.2 installed, how do I compile with an older version?

[pedrocr]

If you're using rustup it's something like:

$ rustup toolchain install 1.22.0
$ cargo +1.22.0 build

If you're not using rustup you'll have to follow your own method.

This PR still has a bunch of commits, the idea would be to squash into a single one.

[johannesvollmer]

$ rustup toolchain install 1.22.0 $ cargo +1.22.0 build

Cool thanks. It's so simple. Rust is awesome <3

[johannesvollmer]

Trying to compile my fork with 1.22.0 yields an error in src/lib.rs, which is probably unrelated to my changes:

not all trait items implemented, missing: description --> src/lib.rs:85:1      | 85 | / impl Error for RawLoaderError { 86 | | }      | |_^ missing description in implementation      |      = note: description from trait: fn(&Self) -> &str

[johannesvollmer]

I squashed the commits

[pedrocr]

I've fixed that issue with Error now.

[johannesvollmer]

Cool. Is there a reason not to specify the Rust version 1.22.0 in travis.yml?

[johannesvollmer]

My code compiles with 1.22.0, I think I made all the requested changes

[pedrocr]

I don't really require 1.22, it's just that right now I've been doing some benchmarking of rust versions so I care that 1.22 keeps works. In general I just care for the latest version.

[pedrocr]

The commit name is not particularly good. Saying that there were squashed commits is not needed and it mentions Debug instead of Clone. Just give it a more generic name like "Style cleanups and derive Clone" or something like that.

[johannesvollmer]

This whole Pull Request just escalated way too much, the changes are soo minimal but this comment section explodes. Haha. I shouldn't code anymore when I'm tired. I'll try to fix the commit message

[johannesvollmer]

Anyways, thank you for bearing with me, and sorry for all the inconveniences. I'll try to use the IRC more often before posting uneducated pull requests. Is everything ok now with this PR?

[pedrocr]

You seem to have pushed a second commit with changes that were already in the repo and a way too generic name.

[johannesvollmer]

Dear God No

[johannesvollmer]

I'll quit coding and go plant some trees instead

[johannesvollmer]

Ah okay, the act of me merging your commits (where you fixed the Error implementation) is actually a separate commit by me, so I have to squash that commit onto the previously squashed commit

[johannesvollmer]

The changes that you made to fix the Error implementation are now also marked as changes in this PR. That doesn't seem correct to me

[pedrocr]

I think the only thing left here was the derive of Clone so I fixed that manually to not have to deal with the merge conflicts. Thanks for going through this.

[johannesvollmer]

Haha nice thanks :D