Removes a left-over --cxxopt=-D style definition that was inert. - #300
Open
zzcgumn wants to merge 3 commits into
Open
Removes a left-over --cxxopt=-D style definition that was inert.#300zzcgumn wants to merge 3 commits into
--cxxopt=-D style definition that was inert.#300zzcgumn wants to merge 3 commits into
Conversation
context_equivalence_test_ctx_tt hardcoded local_defines instead of going through the --define=tt_context_ownership=true config_setting mechanism, and the define is inert (no source #ifdefs it). Remove the target and update the spec accordingly. Co-Authored-By: Claude <noreply@anthropic.com>
DDS_TT_CONTEXT_OWNERSHIP had no source #ifdef and, after removing the last target that hardcoded it, nothing enabled it either. Drop the config_setting, its DDS_LOCAL_DEFINES select() branch, and the spec mentions — TT ownership is unconditionally instance-scoped via SearchContext, so there's nothing left for this flag to gate. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the unused/inert tt_context_ownership build flag plumbing (DDS_TT_CONTEXT_OWNERSHIP) and the corresponding Bazel test target that attempted to enable it, while updating the design/build specs to reflect that TT ownership is always instance-scoped via SearchContext.
Changes:
- Remove
tt_context_ownershipconfig_settingfrom the rootBUILD.bazeland its propagation fromCPPVARIABLES.bzl. - Delete the redundant
context_equivalence_test_ctx_ttsystem test target that only differed bylocal_defines = ["DDS_TT_CONTEXT_OWNERSHIP"]. - Update specs to remove/clarify documentation around the now-removed inert flag.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
specs/transposition-table.md |
Removes mention of the inert ownership flag and clarifies TT ownership behavior. |
specs/build-system.md |
Removes tt_context_ownership from the documented --define feature-flag table. |
library/tests/system/BUILD.bazel |
Drops the context_equivalence_test_ctx_tt test target that had no effect. |
CPPVARIABLES.bzl |
Removes the DDS_TT_CONTEXT_OWNERSHIP define from DDS_LOCAL_DEFINES selection. |
BUILD.bazel |
Removes the tt_context_ownership config_setting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ged a long time ago.
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
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.
Closes #197