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

[QoL][IR] Provide default constructor for NameSupply/GlobalVarSupply #17135

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Lunderberg
Copy link
Contributor

Prior to this commit, a tvm::NameSupply needed to be constructed with an explicit const String& prefix argument. Omitting this argument would fall back to the default constructor provided by the TVM_DEFINE_MUTABLE_OBJECT_REF_METHODS macro, producing a NameSupply holding a nullptr. This then leads to a segfault when the null NameSupply is used.

The vast majority of usages of NameSupply::NameSupply (29 out of 31) initialize it with an empty prefix string. The remaining two use cases initialize it with a non-empty prefix string. There are no cases in which a null NameSupply is initialized.

This commit updates NameSupply to use the TVM_DEFINE_MUTABLE_NOTNULLABLE_OBJECT_REF_METHODS macro instead of TVM_DEFINE_MUTABLE_OBJECT_REF_METHODS. This allows the default constructor to provide the common usage of a NameSupply with an empty prefix, rather than the error-prone usage of a null NameSupply

A similar change is also made for GlobalVarSupply, as the majority of its uses also default to an empty prefix (11 out of 13).

Prior to this commit, a `tvm::NameSupply` needed to be constructed
with an explicit `const String& prefix` argument.  Omitting this
argument would fall back to the default constructor provided by the
`TVM_DEFINE_MUTABLE_OBJECT_REF_METHODS` macro, producing a
`NameSupply` holding a nullptr.  This then leads to a segfault when
the null `NameSupply` is used.

The vast majority of usages of `NameSupply::NameSupply` (29 out of 31)
initialize it with an empty `prefix` string.  The remaining two use
cases initialize it with a non-empty `prefix` string.  There are no
cases in which a null `NameSupply` is initialized.

This commit updates `NameSupply` to use the
`TVM_DEFINE_MUTABLE_NOTNULLABLE_OBJECT_REF_METHODS` macro instead of
`TVM_DEFINE_MUTABLE_OBJECT_REF_METHODS`.  This allows the default
constructor to provide the common usage of a `NameSupply` with an
empty prefix, rather than the error-prone usage of a null `NameSupply`

A similar change is also made for `GlobalVarSupply`, as the majority
of its uses also default to an empty prefix (11 out of 13).
@Lunderberg
Copy link
Contributor Author

I've accidentally made this mistake too many times, and wanted to make it harder to hit this segfault.

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.

None yet

1 participant