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

Fix some style details in generation #1423

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

AThousandShips
Copy link
Member

Some minor output fixes that I came across while fixing some other bugs, this would generate things like:

static Object  *instance_from_id(int64_t instance_id);
Object * get_object() const;
virtual Error _get_packet(const uint8_t * *r_buffer, int32_t *r_buffer_size);

These are all now fixed

@AThousandShips AThousandShips added bug This has been identified as a bug topic:buildsystem Related to the buildsystem or CI setup labels Mar 27, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Mar 27, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner March 27, 2024 13:39
@dsnopek
Copy link
Contributor

dsnopek commented Apr 9, 2024

Thanks!

In my testing this fixes some of the style issues from the description, but creates some new ones.

For example, with MultiplayerPeerExtensnion::_get_packet(), it fixes the const uint8_t * *r_buffer turning it into const uint8_t **r_buffer, but it messes up the next argument, turning it from int32_t *r_buffer_size into int32_t*r_buffer_size (with no spaces at all).

I diff'd the generated code between master and this PR (rebased on master), and skimming through it, it's possible this one "pattern" is the only new issue created.

Here's the diff I generated, in case it's helpful: godot-cpp-pr-1423-diff.txt

@AThousandShips
Copy link
Member Author

Thank you, will look at those cases, should be straightforward

@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 14, 2024

Should be good now!

(Pushed on top of #1490 for the time being for conflicts)

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! All the changes this makes look good to me now :-)

There is at least one style issue this doesn't fix which I noticed while reviewing. I did a diff on the gen/ directory for before and after the last commit on this branch and noticed a missing space after const in a couple places, for example:

diff -Nur gen-old/src/variant/signal.cpp gen/src/variant/signal.cpp
--- gen-old/src/variant/signal.cpp      2024-06-17 16:32:02.756419960 -0500
+++ gen/src/variant/signal.cpp  2024-06-17 16:32:07.316546358 -0500
@@ -117,7 +117,7 @@
        return internal::_call_builtin_method_ptr_ret<int8_t>(_method_bindings.method_is_null, (GDExtensionTypePtr)&opaque);
 }
 
-Object  *Signal::get_object() const{
+Object *Signal::get_object() const{
        return internal::_call_builtin_method_ptr_ret_obj<Object>(_method_bindings.method_get_object, (GDExtensionTypePtr)&opaque);
 }

However, the changes this PR makes are all positive, so we don't have to fix that here (unless you want to).

@AThousandShips
Copy link
Member Author

I'll see if I can find those, but I'm fine merging this and checking later, I'll push in a few hours if I do, otherwise I'll do it later

@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 18, 2024

Fixed!

There are some other areas of style that might be improved in the future, will do some testing using clang-format later but I think these will do well for the time being

Edit: Found a few more glaring cases I'll fix right now

@@ -1545,8 +1549,6 @@ def generate_engine_class_header(class_api, used_classes, fully_used_classes, us
vararg = "is_vararg" in method and method["is_vararg"]

method_signature = "\t"
if vararg:
method_signature += "private: "
Copy link
Member Author

Choose a reason for hiding this comment

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

This is already added by the make_signature method so was added doubly

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! I skimmed through the diff of the generated code after the latest changes, and this is looking really great to me :-)

@dsnopek dsnopek merged commit 0efc6cd into godotengine:master Jun 18, 2024
12 checks passed
@AThousandShips AThousandShips deleted the style_fix branch June 18, 2024 16:25
@AThousandShips
Copy link
Member Author

Thank you! Got some further fixes set up but still testing them and will open a PR when they are ready, they're more extensive but will (almost) make the generared code follow the style, i.e. won't be triggering anything in clang-format

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug cherrypick:4.1 cherrypick:4.2 topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants