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

Rename Output to OutputFileType and deprecated Output #6568

Merged
merged 6 commits into from
Jan 25, 2022

Conversation

abadams
Copy link
Member

@abadams abadams commented Jan 19, 2022

It would be nice in future if we could promote Generator::Input and
Generator::Output to the Halide namespace. For that to happen, we need
to rename the enum Output first. This PR adds a new name for it, and
deprecates the old name.

Not married to the name OutputFile. Other suggestions welcome.

It would be nice in future if we could promote Generator::Input and
Generator::Output to the Halide namespace. For that to happen, we need
to rename the enum Output first. This adds a new name for it, and
deprecates the old name.
Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

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

Should update the Python versions of these enums to use the same nomenclature, see PyEnums.cpp

Re the name, OutputFile is ok with me, but also maybe OutputFileType or OutputType?

src/Module.h Outdated
@@ -43,6 +43,26 @@ enum class Output {
stmt_html,
};

class [[deprecated("Use OutputFile instead of Output")]] Output {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we should still prefer to use HALIDE_ATTRIBUTE_DEPRECATED() here, as it allows downstream folks to disable it via HALIDE_ALLOW_DEPRECATED

Also fix python bindings and use the right deprecation macro
@zvookin
Copy link
Member

zvookin commented Jan 19, 2022

I hate to ask this, and feel free to ignore if it doesn't make sense... The word "type" is heavily overloaded in a compiler project to begin with and I've come to think being careful around its use is a good idea. Specifically trying to use other words when one is not actually talking about a type in either the source or implementation language. E.g. an "output type" would be the type (buffer of ints or something) of an output.

To that end, maybe "OutputKind" is better. We're already using Kind in some places.

This is not a thing I'd object to the PR over, but figured I'd mention it in case improving our words and consistent use of them seems like a thing to do here.

@steven-johnson
Copy link
Contributor

To that end, maybe "OutputKind" is better. We're already using Kind in some places.

SGTM, +1

@steven-johnson
Copy link
Contributor

LGTM

@steven-johnson
Copy link
Contributor

Win32 is apparently unrelated (see #6581 for hopeful fix), LGTM to land once green

@steven-johnson steven-johnson changed the title Rename Output to OutputFile and deprecated Output Rename Output to OutputFileType and deprecated Output Jan 25, 2022
@steven-johnson steven-johnson merged commit 4030a55 into master Jan 25, 2022
@steven-johnson steven-johnson deleted the abadams/deprecate_output branch January 25, 2022 20:21
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.

3 participants