Skip to content

Fixing error: "member may not be initialized" due to constexpr at Windows #48836

Closed
datumbox wants to merge 4 commits intopytorch:masterfrom
datumbox:bugfix/moving_initializer
Closed

Fixing error: "member may not be initialized" due to constexpr at Windows #48836
datumbox wants to merge 4 commits intopytorch:masterfrom
datumbox:bugfix/moving_initializer

Conversation

@datumbox
Copy link
Copy Markdown
Contributor

@datumbox datumbox commented Dec 4, 2020 •
edited
Loading

Fixes #48835
Fixes #48716

@datumbox datumbox requested a review from ezyang December 4, 2020 12:03
@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Dec 4, 2020
@datumbox datumbox mentioned this pull request Dec 4, 2020
Closed
5 tasks
@dr-ci
Copy link
Copy Markdown

dr-ci bot commented Dec 4, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit ec359a7 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

1 failure confirmed as flaky and can be ignored:

  • pytorch_windows_vs2019_py36_cuda10.1_test1
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 11 times.

@datumbox datumbox changed the title Moving initialization outside the class. Fixing error: "member may not be initialized" due to constexpr at Windows Dec 4, 2020
ezyang
ezyang reviewed Dec 4, 2020
View reviewed changes
torch/csrc/jit/serialization/pickler.h
// explicitly instantiate the template, i.e. push<int>(int) works, push(int)
// does not)
static constexpr size_t kBufferSize = 256;
static CONSTEXPR_EXCEPT_WIN_CUDA size_t kBufferSize = 256;
Copy link
Copy Markdown
Contributor

@ezyang ezyang Dec 4, 2020

Choose a reason for hiding this comment

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

Are you sure the same treatment as above isn't needed here?

Copy link
Copy Markdown
Contributor Author

@datumbox datumbox Dec 4, 2020

Choose a reason for hiding this comment

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

This did not seem to have caused any issues. I can check the tests and if I get any indication I can give it the same treatment.

ezyang
ezyang approved these changes Dec 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

Seems worth commenting why constexpr can't be used in ir.h, and also worth commenting that ::Kind cannot be relied on statically (whereas previously it could).

facebook-github-bot
facebook-github-bot reviewed Dec 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@datumbox has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@malfet malfet added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Dec 4, 2020
@datumbox
Copy link
Copy Markdown
Contributor Author

datumbox commented Dec 4, 2020

CI tests fail due to CircleCI outage.

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 4, 2020

Codecov Report

Merging #48836 (ec359a7) into master (eb43e12) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #48836      +/-   ##
==========================================
- Coverage   80.78%   80.78%   -0.01%     
==========================================
  Files        1866     1866              
  Lines      201387   201387              
==========================================
- Hits       162697   162691       -6     
- Misses      38690    38696       +6

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Dec 6, 2020

@datumbox merged this pull request in e429d05.

@datumbox datumbox deleted the bugfix/moving_initializer branch December 7, 2020 20:15
@ezyang ezyang mentioned this pull request Jan 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang approved these changes

+1 more reviewer

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

4 participants

@datumbox @facebook-github-bot @ezyang @malfet