Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define all C++ model constructors explicit #2944

Merged
merged 2 commits into from
Nov 3, 2020

Conversation

datumbox
Copy link
Contributor

@datumbox datumbox commented Oct 31, 2020

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 conversions

By allowing implicit calls the following misleading usage is possible:
before

This can be stopped by defining all the constructors that receive up to 1 mandatory parameter as explicit:
after

After the change, the following 2 initializations are valid:

DenseNetImpl x1(100);
DenseNetImpl x2{100};

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_75 and test_mnasnet1_3 which also fail on master.

@datumbox datumbox requested a review from fmassa October 31, 2020 13:19
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 31, 2020

With explicit specifier we prevent of using copy initialization: no more of

DenseNetImpl net = 1000; // error: copy-list-initialization

@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

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

Impacted file tree graph

@@           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.

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

@datumbox
Copy link
Contributor Author

datumbox commented Oct 31, 2020

@vfdev-5 we could still do:

DenseNetImpl z{100};
DenseNetImpl x = z;

But as you said this will no longer be not allowed:

DenseNetImpl x = 100;

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. :)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 31, 2020

@datumbox I totally agree ! Definition like that DenseNetImpl x = 100; does not make much sense.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot Vasilis!

@fmassa fmassa merged commit 6b071be into pytorch:master Nov 3, 2020
@datumbox datumbox deleted the refactor/explicit_constructors branch November 3, 2020 10:44
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* Making all model constructors explicit.

* formatting.
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* Making all model constructors explicit.

* formatting.
@datumbox datumbox mentioned this pull request Jan 5, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants