Define all C++ model constructors explicit#2944
Conversation
|
With explicit specifier we prevent of using copy initialization: no more of |
Codecov Report
@@ Coverage Diff @@
## master #2944 +/- ##
=======================================
Coverage 73.41% 73.41%
=======================================
Files 99 99
Lines 8801 8801
Branches 1389 1389
=======================================
Hits 6461 6461
Misses 1915 1915
Partials 425 425 Continue to review full report at Codecov.
|
|
@vfdev-5 we could still do: But as you said this will no longer be not allowed: More specifically what we don't allow the compiler to do is silently take the integer 100, pass it on the constructor as first argument to create a temporary object and then copy to x (thought optimizations can happen here by compiler). I think it's better to not allow this syntax because it leads to less readable code and potential bugs. Hence it's by default a warning on clang tidy. :) |
|
@datumbox I totally agree ! Definition like that |
* Making all model constructors explicit. * formatting.
* Making all model constructors explicit. * formatting.
4 participants
Static analysis on the C++ code at
csrc/models/produces the following warning:Clang-Tidy: Constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversionsBy allowing implicit calls the following misleading usage is possible:
This can be stopped by defining all the constructors that receive up to 1 mandatory parameter as explicit:
After the change, the following 2 initializations are valid:
Note that thought this might be considered theoretically a breaking change on the C++ API, I believe not many users use the old initialization as it's quite misleading.
I tested the changes by manually running the test_cpp_models.py unit-tests. All pass except of
test_mnasnet0_5,test_mnasnet0_75andtest_mnasnet1_3which also fail on master.