search

microsoft / knossos-ksc

Compiler with automatic differentiation
Other
45 stars 10 forks source link

Fix map on tuples #848

Closed toelli-msft closed 3 years ago

toelli-msft commented 3 years ago

... and simplify $check into the bargain.

Fixes https://github.com/microsoft/knossos-ksc/issues/847

It would be good to have the C++ experts check that this makes sense.

dcrc2 commented 3 years ago

It would be good to have the C++ experts check that this makes sense.

Ah, now if I was a real C++ expert, I'd be able to explain whether it's valid to use applyWithAllocator in map here, before applyWithAllocator has been defined. I think it's fine, but the rules about this are notoriously obscure. I'd suggest avoiding all this by moving applyWithAllocator above map, perhaps at the end of the "Allocator" section.

toelli-msft commented 3 years ago

I'd suggest avoiding all this by moving applyWithAllocator above map

Given that it works as written is there any strong need to do this? Is it liable to break in hard to diagnose ways if I don't, perhaps?

awf commented 3 years ago

I'd suggest avoiding all this by moving applyWithAllocator above map

Given that it works as written is there any strong need to do this? Is it liable to break in hard to diagnose ways if I don't, perhaps?

Seems like a sensible suggestion to avoid anything that might be "notoriously obscure".

dcrc2 commented 3 years ago

I'd suggest avoiding all this by moving applyWithAllocator above map

Given that it works as written is there any strong need to do this? Is it liable to break in hard to diagnose ways if I don't, perhaps?

For this particular function I think you're fine. There is similar code which does break, for example if you decided to specify the namespace explicitly (ks::applyWithAllocator), or if instead of applyWithAllocator you had a function which required an explicit template argument (such as build), or if you were referring to a type rather than a function.

But I think the convention is to order to contents of headers to avoid referring to functions defined later in the file, because sometimes this is necessary, even though it isn't necessary in this case.

awf commented 3 years ago

But I think the convention is to order to contents of headers to avoid referring to functions defined later in the file, because sometimes this is necessary, even though it isn't necessary in this case.

Indeed, and to use forward declarations otherwise, either for mutual recursion or just for clarity of exposition.

toelli-msft commented 3 years ago

OK, sounds like a good idea to avoid anything fragile so I enacted David's suggestion. Will merge imminently unless anyone shouts.