Why a copy?

In particular, getAllOperators() makes me think that whatever you're going to do once you've gotten them all should probably just be a method on OperatorRegistry…since that's the thing we have that represents the set of all operators.

can we implement this with SmallVector as discussed?

We could... I think part of the reason of using an existing implementation is to avoid the cognitive overhead of rethinking & retesting it. The more you change the implementation the less that applies

I would argue that you should be rethinking and retesting code that you are copying from elsewhere. Like, maybe for a templated mess like optional it's not worth, but this function isn't so complicated

Please follow fb standards:

• capitalize "compute"
• Move the impl to a cpp file. The current way you're doing it will create ODR violations if we try to include this file from multiple TUs.

maybe static constexpr?

nit: split this into multiple lines

nit: close_operators.begin()->first seems more readable

Copying every operator name is unnecessary. if computeEditDistance took an ArrayRef or const chart * you can avoid it entirely

It's being passed as a const std::string reference. How is that different than const char * ?

The copy is made in the line I indicated, when you invoke the copy constructor of std::string.

const auto would have copied the const char *, which is just a pointer that you could then pass to computeEditDistance without a copy.

I don't think you need select and rank the operators in two different functions. Why can't there be a single function that returns the n closest ops? Doing that with a priority queue with the comparator being edit distance is a more explicit way to accomplish what you're doing here.

why std::multimap over std::vector? It looks like the keys aren't actually used anywhere.

So that it prints out the closer suggestions first

formatting

this comment appears outdated

all of these should be const

const

You're passing around a very complicated data structure (std::multimap<int64_t, std::pair<Symbol, std::vector<std::shared_ptr<Operator>>>) when you only need std::vector<Symbol>. I don't think it's good to spill the guts of your implementation out like that.

If you want to return them in order, just use a std::priority_queue to maintain ordering and return the underlying container.

what's this for?

This is a type conversion from (int a, int b, int c) -> []int. Just as the other type conversions, it should only be run on the second pass when allow_conversions is true. I think you have an issue raised about this logic IIRC.

I ran into a bug bc of it when testing this.

nice! Can you add a test that checks this to avoid regression in the future?

Hm i think the error actually was in a different PR, the torch.tensor one and accidentally made it's way in here. Removing from this PR and adding a test in that one