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

[easy] Address some compilation warnings #178

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Dec 16, 2024

Met some warnings during compilation, this PR fix them all.
Most of the warning here are caused by comparison between signed and unsigned integers.

@@ -22,7 +22,6 @@ namespace core {

ParserExtensionParseResult duckpgq_parse(ParserExtensionInfo *info,
const std::string &query) {
auto parse_info = (DuckPGQParserExtensionInfo &)(info);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable not used anywhere.

@@ -42,7 +42,7 @@ static void LocalClusteringCoefficientFunction(DataChunk &args,

DuckPGQBitmap neighbors(v_size);

for (int32_t n = 0; n < args.size(); n++) {
for (idx_t n = 0; n < args.size(); n++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/home/ubuntu/duckpgq-extension/src/core/functions/scalar/local_clustering_coefficient.cpp:45:25: warning: comparison of integer expressions of different signedness: ‘int32_t’ {aka ‘int’} and ‘duckdb::idx_t’ {aka ‘long unsigned int’} [-Wsign-compare]
   45 |   for (int32_t n = 0; n < args.size(); n++) {
      |                       ~~^~~~~~~~~~~~~

@@ -54,15 +54,15 @@ static void LocalClusteringCoefficientFunction(DataChunk &args,
continue;
}
neighbors.reset();
for (size_t offset = v[src_node]; offset < v[src_node + 1]; offset++) {
for (int64_t offset = v[src_node]; offset < v[src_node + 1]; offset++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

**/home/ubuntu/duckpgq-extension/src/core/functions/scalar/local_clustering_coefficient.cpp:57:46: warning: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int64_t’ {aka ‘long int’} [-Wsign-compare]
   57 |     for (size_t offset = v[src_node]; offset < v[src_node + 1]; offset++) {
      |                                       ~~~~~~~^~~~~~~~~~~~~~~~~**

@Dtenwolde
Copy link
Contributor

Thanks for the PR!
Regarding the DuckPGQParserExtensionInfo, I don't see any use for it in the repo. I think it is an artefact of the early days of the extension when I wasn't quite sure if we needed to use this. Since there are no use cases, I think this entire struct should be removed to avoid having redundant code in the repo.

@dentiny
Copy link
Contributor Author

dentiny commented Dec 16, 2024

Searching through the codebase, it's actually used in one place:

parser_info = make_shared_ptr<DuckPGQParserExtensionInfo>();

which seems be a necessary field for ParserExtension: https://github.com/cwida/duckdb-pgq/blob/0c82f0ed582ed2737216539845028826badb4bda/src/include/duckdb/parser/parser_extension.hpp#L82-L94

May I double confirm if it's completely removable?

@Dtenwolde
Copy link
Contributor

I commented out the struct and any uses, and all tests passed; however, maybe it's best to just leave it in anyway. Sorry for the confusion

@Dtenwolde Dtenwolde merged commit e5e624d into cwida:main Dec 17, 2024
35 checks passed
@dentiny
Copy link
Contributor Author

dentiny commented Dec 18, 2024

Thank you for spending time taking a look for me!

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.

2 participants