Skip to content

Rename Output to OutputFileType and deprecated Output#6568

Merged
steven-johnson merged 6 commits into
masterfrom
abadams/deprecate_output
Jan 25, 2022
Merged

Rename Output to OutputFileType and deprecated Output#6568
steven-johnson merged 6 commits into
masterfrom
abadams/deprecate_output

Conversation

@abadams

@abadams abadams commented Jan 19, 2022

Copy link
Copy Markdown
Member

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.

@steven-johnson steven-johnson left a comment

Copy link
Copy Markdown
Contributor

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?

Comment thread src/Module.h Outdated
stmt_html,
};

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

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

zvookin commented Jan 19, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor

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

SGTM, +1

@steven-johnson steven-johnson self-requested a review January 21, 2022 22:25
@steven-johnson

Copy link
Copy Markdown
Contributor

LGTM

@steven-johnson

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