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

Make generated code mostly style compliant #1528

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jul 17, 2024

Follow-up to:

This makes running clang-format on the generated files change almost nothing, the only things this does not do correctly is:

  • Align continuations in class_db_singleton.hpp, which would be a lot of extra code to make work correctly
  • Some cases with public: followed immediately by private: should have no empty line (or no public: in the first place), this could be fixed but only occurs in one place, so leaving it be for right now and not adding any complicating code for it

@AThousandShips AThousandShips added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup cherrypick:4.1 cherrypick:4.2 labels Jul 17, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Jul 17, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner July 17, 2024 14:32
@dsnopek
Copy link
Collaborator

dsnopek commented Jul 17, 2024

Thanks!

I diffed the gen/ directory from before and after this PR, and skimmed the result. For the most part, it looks really great!

I did notice one odd change:

-       return internal::_call_builtin_operator_ptr<int8_t>(_method_bindings.operator_not, (GDExtensionConstTypePtr)&opaque, (GDExtensionConstTypePtr)nullptr);
+       return internal::_call_builtin_operator_ptr<int8_t>(_method_bindings.operator_not, (GDExtensionConstTypePtr)&opaque, (GDExtensionConstTypePtr) nullptr);

It's adding a space before nullptr for some reason.

@AThousandShips
Copy link
Member Author

Will take a look, realized there were some potentially breaking things so will check first, making draft atm

@AThousandShips AThousandShips marked this pull request as draft July 17, 2024 15:19
@AThousandShips
Copy link
Member Author

It's adding a space before nullptr for some reason.

This is intentional, it's compliant with clang-format

Fixed the error, will take a look over it again to make sure I didn't break anything and push soon, and adding some further improvements

@AThousandShips AThousandShips marked this pull request as ready for review July 17, 2024 15:49
@AThousandShips
Copy link
Member Author

Will see if I can adjust some minor issues, like empty public: blocks, and the alignment of continuations, but now it generates correctly

To confirm this doesn't break things do:

git checkout master
scons generate_bindings=Yes build_library=No
clang-format -i gen/include/godot_cpp/classes/*.hpp gen/include/godot_cpp/variant/*.hpp gen/src/classes/*.hpp gen/src/variant/*.cpp 
git add -f gen/*

then checkout this, run

scons generate_bindings=Yes build_library=No

And compare the diffs, it should now just be minor style differences, and no lost code (accidentally removed some private: cases before

@dsnopek
Copy link
Collaborator

dsnopek commented Aug 8, 2024

Sorry for taking so long to come back this!

It's adding a space before nullptr for some reason.

This is intentional, it's compliant with clang-format

Ah, ok, I didn't know that.

I skimmed through the diff between the code generated before and after the latest version of this PR, and I didn't spot any issues, although, it is a very long diff. :-)

So, this looks good to me, but I think I'll hold off on actually merging it until after Godot 4.3-stable is released, just in case.

@dsnopek dsnopek merged commit 8b80d91 into godotengine:master Aug 21, 2024
12 checks passed
@AThousandShips AThousandShips deleted the style_fix_2 branch August 21, 2024 12:40
@dsnopek
Copy link
Collaborator

dsnopek commented Sep 3, 2024

Cherry-picked for 4.2 in PR #1570

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 4, 2024

This one won't cleanly cherry-pick to the 4.1 branch - I think it'd need a custom PR for it. However, given that 4.3 is the current stable now, I'm not entirely sure it makes sense to put in the effort. But we still need to decide on the support plan for older versions in general, so I'm going to leave the cherrypick:4.1 label in place for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants