query-engine-c-abi: Fix build warnings after update #4864
Merged
Conversation
This file contains 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
`rustc` now detects unused trait methods, which means we need to make a new offering to the gods of `cfg[feature=]`.
CodSpeed Performance ReportMerging #4864 will not alter performanceComparing Summary
|
WASM Query Engine file Size
|
✅ WASM query-engine performance won't change substantially (1.000x)Full benchmark reportAfter changes in 4dab8ff |
Druue
reviewed
May 10, 2024
| @@ -82,6 +82,7 @@ impl TypeIdentifier for Column<'_> { | |||
| ) | |||
| } | |||
|
|
|||
| #[cfg(feature = "mysql")] | |||
There was a problem hiding this comment.
this feels really weird that a mysql feature is in sqlite; am I missing something?
There was a problem hiding this comment.
I think TypeIdentifier is used only by sqlite and mysql. So, if mysql is enabled, method in trait will exist and that will force us to implement one for sqlite too.
There was a problem hiding this comment.
Right but we also have the same file for mysql. That feels like it'd make more sense for us to tag this one as feature = "sqlite" and then add the same fn over in the mysql connector
There was a problem hiding this comment.
This will break the build where sqlite and mysql are always included.
Situation is:
- if
mysqlis not enabled thanTypeIdentifierwill not haveis_jsonmethod and it all will be good - If
sqliteis not enabled, all methods are used and there will be no warnings. - If
mysqlis enabled,is_jsonmethod will be added to trait and sincesqliteandmysqlshare the trait we'll be forced to implement it for sqlite as well.
|
I am gonna merge it since i need a RN release, but fell free to continue the discussion, if any changes are needed, I'll open a follow up PR |
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.
rustcnow detects unused trait methods, which means we need to make anew offering to the gods of
cfg[feature=].