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

Redeclare additional *_walker fn #1629

Merged

Conversation

daamien
Copy link
Contributor

@daamien daamien commented Apr 2, 2024

As a follow up to the discussion here
#1583 (comment)

Here's a patch to declare correctly the following functions:

  • query_or_expression_tree_walker
  • range_table_entry_walker
  • planstate_tree_walker
  • range_table_walker

This is a shameless copy of PR #1596 , I'm not entirely sure what I am actually doing here :)

Quick questions regarding the format:

  • I see some arguments have the arg_ prefix while others don't... Like arg_node vs node in the query_tree_walker function... Is there a naming rule regarding this ?

  • I'm unsure how to sort the functions... alphabetical order ?

@daamien daamien force-pushed the fix-more-tree-walker-fn branch from 7f1aea2 to ac52c3f Compare April 2, 2024 14:17
@workingjubilee
Copy link
Member

I see some arguments have the arg_ prefix while others don't... Like arg_node vs node in the query_tree_walker function... Is there a naming rule regarding this ?

We should generally copy the names from the C source except when they deviate from Rust convention. I'm not sure why raw_tree_expression_walker is like that, it's a deviation from that rule. Probably should be fixed.

I'm unsure how to sort the functions... alphabetical order ?

That will do fine for now.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Would you be so kind as to make sure the arg names line up with the relevant _impl functions? (including in previously-added fn)

pgrx-pg-sys/build.rs Outdated Show resolved Hide resolved
@daamien
Copy link
Contributor Author

daamien commented Apr 4, 2024

Would you be so kind as to make sure the arg names line up with the relevant _impl functions? (including in previously-added fn)

Sure !

Sorry I misunderstood your previous answer..... I will rename the arguments...

@workingjubilee
Copy link
Member

I think all the ones you added here are correct?

@workingjubilee workingjubilee merged commit 474f0c6 into pgcentralfoundation:develop Apr 5, 2024
11 checks passed
@daamien daamien deleted the fix-more-tree-walker-fn branch September 23, 2024 19:47
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