[Cleanup] Remove using namespace tvm::runtime from headers#17246
Merged
tqchen merged 1 commit intoapache:mainfrom Aug 22, 2024
Merged
[Cleanup] Remove using namespace tvm::runtime from headers#17246tqchen merged 1 commit intoapache:mainfrom
using namespace tvm::runtime from headers#17246tqchen merged 1 commit intoapache:mainfrom
Conversation
Lunderberg
added a commit
to Lunderberg/tvm
that referenced
this pull request
Aug 6, 2024
Re-apply the change from upstream apache#16343. See apache#17246 for an example of the type of bug that can result from MSVC's default non-standard name resolution.
tqchen
approved these changes
Aug 6, 2024
tqchen
requested changes
Aug 7, 2024
Member
tqchen
left a comment
tqchen
left a comment
There was a problem hiding this comment.
temp block this in light of #16183 (comment)
Contributor
Author
Good point. I think this change is beneficial regardless of #16183, but the FFI changes touch so many parts of TVM that I wouldn't want to declare them 100% independent. |
Prior to this commit, various header files had `using namespace tvm::runtime`, which imports all names from `tvm::runtime` into the current namespace. These imports can cause compilation errors depending on the order of `#include` statements. For example, the `#include <tvm/relay/attrs/transform.h>` file uses the unqualified name `Bool` to refer to `::tvm::Bool`, a subclass of `PrimExpr`. If a different header file specifies `using namespace tvm::runtime` within the `tvm::relay` namespace, then the unqualified name `Bool` ambiguously refers to either `::tvm::Bool` or `::tvm::runtime::Bool`. In MSVC, this can cause even further compilation errors. By default, MSVC does not follow the C++ standard for name resolution in templates. The standard requires that any names in a template that do not depend on template parameters be resolved when the template is declared. However, MSVC instead resolves these names when the template is instantiated. As a result, the same `using namespace tvm::runtime` may cause a compilation error if it occurs after the template's declaration, but before the template's usage. (TVM provides the `/permissive-` flag to MSVC builds specifically to disable MSVC's non-standard name resolution, so this only impacts downstream forks that disable this flag. See apache#16343 for more details.) This commit removes `using namespace tvm::runtime`, replacing them with explicit `using tvm::runtime::SOME_SPECIFIC_SYMBOL` where necessary. This resolves both the include-order dependency for standards-compliant compilers, and the compilation errors for MSVC's default build.
Lunderberg
force-pushed
the
cleanup_remove_using_namespace_tvm_runtime
branch
from
757518d to
4a01e78
Compare
Contributor
Author
|
Rebased these changes onto main, and this PR should be land-able now that the performance regressions from #16183 have been resolved. |
tqchen
approved these changes
Aug 22, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prior to this commit, various header files had
using namespace tvm::runtime, which imports all names fromtvm::runtimeinto the current namespace.These imports can cause compilation errors depending on the order of
#includestatements. For example, the#include <tvm/relay/attrs/transform.h>file uses the unqualified nameBoolto refer to::tvm::Bool, a subclass ofPrimExpr. If a different header file specifiesusing namespace tvm::runtimewithin thetvm::relaynamespace, then the unqualified nameBoolambiguously refers to either::tvm::Boolor::tvm::runtime::Bool.In MSVC, this can cause even further compilation errors. By default, MSVC does not follow the C++ standard for name resolution in templates. The standard requires that any names in a template that do not depend on template parameters be resolved when the template is declared. However, MSVC instead resolves these names when the template is instantiated. As a result, the same
using namespace tvm::runtimemay cause a compilation error if it occurs after the template's declaration, but before the template's usage.(TVM provides the
/permissive-flag to MSVC builds specifically to disable MSVC's non-standard name resolution, so this only impacts downstream forks that disable this flag. See #16343 for more details.)This commit removes
using namespace tvm::runtime, replacing them with explicitusing tvm::runtime::SOME_SPECIFIC_SYMBOLwhere necessary. This resolves both the include-order dependency for standards-compliant compilers, and the compilation errors for MSVC's default build.