[GitHub] sifmelcara commented on issue #9809: fix optimizer bug in CPP-Package
sifmelcara commented on issue #9809: fix optimizer bug in CPP-Package URL: https://github.com/apache/incubator-mxnet/pull/9809#issuecomment-367824559 Hmm, in fact I think adding back `static int __make_ ## OptimizerType ## _ ## Name ## __ =` is a little bit better because local static variable's initialization is guaranteed to be thread safe. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sifmelcara commented on issue #9809: fix optimizer bug in CPP-Package
sifmelcara commented on issue #9809: fix optimizer bug in CPP-Package URL: https://github.com/apache/incubator-mxnet/pull/9809#issuecomment-367822502 Hello! Just got a ping from @chsin for this PR. I'm the author of the ODR fix. After some git blame, I found this bug was accidentally introduced by https://github.com/apache/incubator-mxnet/commit/1c22bc71052f54eb21a6079eebdc7383dafbb4ee which tries to fix a compiler warning of return value not used... But we need `static int __make_ ## OptimizerType ## _ ## Name ## __ =` for a reason: it is used to make sure optimizers are only registered once. So either add back `static int __make_ ## OptimizerType ## _ ## Name ## __ =` or merge this PR should be able to fix #9800 . However, as @szha mentioned, I wonder if there is a better way to register the optimizers? I guess we can use global inline variable but that would require C++17 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sifmelcara commented on issue #9809: fix optimizer bug in CPP-Package
sifmelcara commented on issue #9809: fix optimizer bug in CPP-Package URL: https://github.com/apache/incubator-mxnet/pull/9809#issuecomment-367822502 Hello! Just got a ping from @chsin for this PR. I'm the author of the ODR fix. After some git blame, I found this bug was accidentally introduced by https://github.com/apache/incubator-mxnet/commit/1c22bc71052f54eb21a6079eebdc7383dafbb4ee which tries to fix a compiler warning of return value not used... But we need `static int __make_ ## OptimizerType ## _ ## Name ## __ =` for a reason, it is used to make sure optimizers are only registered once. So either add back `static int __make_ ## OptimizerType ## _ ## Name ## __ =` or merge this PR should be able to fix #9800 . However, as @szha mentioned, I wonder if there is a better way to register the optimizers? I guess we can use global inline variable but that would require C++17 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sifmelcara commented on issue #9809: fix optimizer bug in CPP-Package
sifmelcara commented on issue #9809: fix optimizer bug in CPP-Package URL: https://github.com/apache/incubator-mxnet/pull/9809#issuecomment-367822502 Hello! Just got a ping from @chsin for this PR. I'm the author of the ODR fix. After some git blame, I found this bug was accidentally introduced by https://github.com/apache/incubator-mxnet/commit/1c22bc71052f54eb21a6079eebdc7383dafbb4ee which tries to fix a compiler warning of return value not used... We need `static int __make_ ## OptimizerType ## _ ## Name ## __ =` to make sure optimizers are only registered once. So either add back `static int __make_ ## OptimizerType ## _ ## Name ## __ =` or merge this PR should be able to fix #9800 . However, as @szha mentioned, I wonder if there is a better way to register the optimizers? I guess we can use global inline variable but that would require C++17 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
