Skip to content

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Feb 6, 2019

Stack:
    :black_circle:  #16824 Mark IntList as deprecated; add C10_DEPRECATED_USING  💛

There was a big wooly yak getting the deprecated macros to work.
Gory details are in Deprecated.h

Differential Revision: D13978429

Differential Revision: D13978429
Differential Version: 71266126
@mhubii
Copy link

mhubii commented Feb 8, 2019

What to use instead of the IntList? @ezyang

@ezyang
Copy link
Contributor Author

ezyang commented Feb 8, 2019

IntArrayRef!

Differential Revision: D13978429
Differential Version: 71704832
#define C10_DEPRECATED(function) function
#endif // defined(__GNUC__)
#endif // defined(__cplusplus) && __cplusplus > 201402L
# warning "You need to implement C10_DEPRECATED_USING for this compiler"
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't quite follow all the cases...are we actually using this? Is it crazy to make this an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't make it an error... everything will "work" in this case, the user will just not get deprecated warnings. Better not break the build for no good reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about the first question? Is this actually used in any of our builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this shouldn't be exercised by any of our builds.

Copy link
Contributor

@smessmer smessmer left a comment

Choose a reason for hiding this comment

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

I'm fine with this if you want to land it, but also have an alternative proposal for you to consider:
Have no split between C10_DEPRECATED and C10_DEPRECATED_USING, but disable C10_DEPRECATED on nvcc. That disables it for a few more cases than your proposal, but makes it easier to use. Also, disabling this in nvcc probably doesn't matter too much because the same code is run through non-nvcc and will show warnings there anyhow.

@ezyang
Copy link
Contributor Author

ezyang commented Feb 11, 2019

@smessmer Your suggestion is reasonable, but I don't want to turn off nvcc support entirely, since that means that we will not get deprecated warnings for any cu code, which can include host code.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Feb 13, 2019
Summary:
Pull Request resolved: pytorch/pytorch#16824

There was a big wooly yak getting the deprecated macros to work.
Gory details are in Deprecated.h

Reviewed By: smessmer

Differential Revision: D13978429

fbshipit-source-id: f148e5935ac36eacc481789d22c7a9443164fe95
@soumith soumith deleted the export-D13978429 branch February 21, 2019 23:22
@ezyang ezyang added the merged label Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants