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

Avoid macro redefinitions and function name collisions between sources #445

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

andrjohns
Copy link
Contributor

When experimenting with a jumbo build for the quickjs sources, I found a few cases of functions/macros with the same name in multiple sources.

This PR simply renames the offending functions and removes unused macros, but it could also be resolved by adding them to the public API for a given header

libbf.c Outdated
@@ -179,10 +179,6 @@ static inline __maybe_unused limb_t shld(limb_t a1, limb_t a0, long shift)
return a1;
}

#define malloc(s) malloc_is_forbidden(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thsi ones are here to prevent accidental allocations, they need to exist. Perhaps we could skip them with some define @chqrlie ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the easiest way to maintain current behaviour would be to just #undef them at the end of the respective source file, so that the (intentional) malloc calls in subsequent sources aren't affected in a jumbo build.

How does that sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

Great! Updated the changes, safe for re-review when you've got a minute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah hold on, was just testing with cmake's -DCMAKE_UNITY_BUILD=ON and there are a few more collisions

Copy link
Contributor Author

@andrjohns andrjohns Jun 27, 2024

Choose a reason for hiding this comment

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

That should be all for now. There are a couple of places where CMakeLists needs to be changed when using -DCMAKE_UNITY_BUILD=ON with -DBUILD_QJS_LIBC=ON but I'll make a separate PR for that

quickjs.c Outdated Show resolved Hide resolved
@saghul saghul merged commit e8ef9d6 into quickjs-ng:master Jun 27, 2024
50 checks passed
@saghul
Copy link
Contributor

saghul commented Jun 27, 2024

Thanks!

@andrjohns andrjohns deleted the redefinitions branch July 1, 2024 13:18
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.

None yet

2 participants