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

Fix deprecation warning in scalar_type_analysis #50218

Conversation

malfet
Copy link
Contributor

@malfet malfet commented Jan 7, 2021

No description provided.

@malfet malfet requested a review from a team January 7, 2021 18:53
@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Jan 7, 2021
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 7, 2021

💊 CI failures summary and remediations

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


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

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 to the (internal) Dr. CI Users group.

This comment has been revised 10 times.

Copy link
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.

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

Copy link
Contributor

@janeyx99 janeyx99 left a comment

Choose a reason for hiding this comment

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

Throughout the codebase, I see invocations of this function called as isIntegralType(t, /*includeBool=*/ false). Why do we have that/should we be consistent here?

@samestep
Copy link
Contributor

samestep commented Jan 7, 2021

Throughout the codebase, I see invocations of this function called as isIntegralType(t, /*includeBool=*/ false). Why do we have that/should we be consistent here?

lol we could do this :P https://www.fluentcpp.com/2018/12/14/named-arguments-cpp/

@malfet malfet force-pushed the malfet/fix-deprecation-warning-in-scalar-type-analysis branch from c88e63b to 2713e16 Compare January 7, 2021 20:37
@malfet
Copy link
Contributor Author

malfet commented Jan 7, 2021

Throughout the codebase, I see invocations of this function called as isIntegralType(t, /*includeBool=*/ false). Why do we have that/should we be consistent here?

Because "false" argument is quite confusing without knowing the function signature. A rather common workaround for this problem is to define enum class includeBool { true, false}; and use it as a type of the 2nd argument.

Copy link
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.

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

@malfet malfet force-pushed the malfet/fix-deprecation-warning-in-scalar-type-analysis branch from 2713e16 to 6d0e518 Compare January 7, 2021 20:49
Copy link
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.

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

@facebook-github-bot
Copy link
Contributor

@malfet merged this pull request in 0495180.

@malfet malfet deleted the malfet/fix-deprecation-warning-in-scalar-type-analysis branch January 7, 2021 22:46
hwangdeyu pushed a commit to hwangdeyu/pytorch that referenced this pull request Jan 14, 2021
Summary: Pull Request resolved: pytorch#50218

Reviewed By: janeyx99

Differential Revision: D25827971

Pulled By: malfet

fbshipit-source-id: a4e467721435d7ae0db2195694053621eee8c2ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants