Skip to content

Conversation

@Novare
Copy link

@Novare Novare commented Jan 15, 2020

This fixes two different errors regarding the reducer functionality in segment.cpp.

First of all, the AT_DISPATCH_REDUCTION_TYPES macro places the declaration of REDUCE in different conditional scopes, which causes the compiler (cl.exe in my case) to fail since REDUCE is then only defined if one of the if scopes is entered. The lambda thrown in as a second argument doesn't seem to be able to catch that variable either. This is easily fixed by defining REDUCE once before differentiating between the types.

Secondly, making the ReductionType a template argument of the Reducer forces us to define the ReductionType at runtime (as a constant expression), which isn't happening, causing another error at compile time. Since the Reducer doesn't have any internal state and all functions only operate on their parameters, we can safely move the ReductionType into the parameter list of the functions. This fixes the second error. As a neat plus we also don't need to create an instance of every possible ReducerType using the template to call the Reducer with every possible type.

@rusty1s
Copy link
Owner

rusty1s commented Jan 15, 2020

I wonder why these errors did not occur under Linux and Mac. Overall, this looks good to me. Can you now successfully install all kernels?

@codecov-io
Copy link

Codecov Report

Merging #100 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #100   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         143    143           
=====================================
  Hits          143    143

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1eabf7f...cd84568. Read the comment docs.

@Novare
Copy link
Author

Novare commented Jan 15, 2020

I wonder why these errors did not occur under Linux and Mac. Overall, this looks good to me.

Same for me. I can see why the compiler would complain about the second thing as template arguments do always need to be constant expressions and REDUCE is decided at runtime. The first one is a mystery to me though - different compilers might handle lambdas capturing symbols differently.

@rusty1s
Copy link
Owner

rusty1s commented Jan 15, 2020

Thanks for your great help. I will merge this :)

@rusty1s rusty1s merged commit 6733f0e into rusty1s:master Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants