macros/class: Fix HashSet contains+inserts pair to only lookup once

[MarijnS95]

Overlooked style nit from https://github.com/microsoft/com-rs/pull/224/files#r732498255 that both makes the code more pretty to read as per the suggestion from @rylev, and by using the boolean result from .insert() no second lookup with .contains() needs to be performed, shaving some cylces off.

---

Test: cargo t --all remains successful with this. Commenting out the .filter() line makes some tests fail.

CC @sivadeilra

[sivadeilra]

I would prefer not to merge this change. I think the changed code is harder to understand, especially for developers who are not accustomed to iterator chains, which is why I did not make this change in my PR. I think the use of ordinary control-flow makes it more obvious what is going on.

[MarijnS95]

@sivadeilra I'd argue that the existing code around code generation is already much more next-level than a simple .filter call.

However, if you insist we should at least teach every developer about insert returning a boolean, writing this instead:

                // Avoid generating duplicate From implementations
                if !interfaces_seen.insert(interface_path) {
                    continue;
                }

That still has your way of ordinary control flow, but should point any developer going "huh, insert() returns a bool??" to the documentation to find out that they don't need to waste time on a contains+insert pair.

Thougts?

[sivadeilra]

I suppose it's fine. Either way. Feel free to merge this PR, either with the .filter() or with the control-flow. I just lean a little away from iterator chains, perhaps more than most Rust developers. (I wish there were a better debugging story for iterator chains, too.)

[MarijnS95]

Let's wait for other maintainers' view on this then.

Personally I've embraced Rust's iterators (and pretty much every other neat feature they come up with), but do agree that iterator chains look out of place inside a for expression.

[rylev]

I much prefer using iterators over traditional control flow mechanisms, but I do think the combination with the fact that insert returns a bool is potentially a bit too clever. However, I would have likely merged this without question if it were not for @sivadeilra's comments. I'll leave this open for any final objections, and then merge if others don't continue to have strong objections.