#1699 Update FreeBSD support

Затворено
отворено пре 5 месеци од OlCe1 · 12 коментара
OlCe1 коментирира пре 5 месеци

Hi,

Just in case you didn’t see the forum message (https://forum.palemoon.org/viewtopic.php?f=5&t=25625&p=204527#p205217).

Please have a look at https://repo.palemoon.org/OlCe1/UXP/commits/branch/FreeBSD-support so we can start on importing the code.

Happy to answer comments/questions.

Regards.

Hi, Just in case you didn't see the forum message (https://forum.palemoon.org/viewtopic.php?f=5&t=25625&p=204527#p205217). Please have a look at https://repo.palemoon.org/OlCe1/UXP/commits/branch/FreeBSD-support so we can start on importing the code. Happy to answer comments/questions. Regards.
mattatobin коментирира пре 5 месеци
Коаутор

There is at least one format error in the last big commit to mozjemalloc and i am extremely sceptical about such a large change and introduction of brand new code to it.

However, I am not qualified to evaluate such a huge change.

The rest looks sane enough.

@Moonchild and @athenian200 what say you?

Once this checks out and is approved I will manually land this to trunk following our commit form which will be the example to follow for you @OlCe1 in the future. So is that ALL that is required?


Compare reference: https://repo.palemoon.org/MoonchildProductions/UXP/compare/master...OlCe1:FreeBSD-support

There is at least one format error in the last big commit to mozjemalloc and i am extremely sceptical about such a large change and introduction of brand new code to it. However, I am not qualified to evaluate such a huge change. The rest looks sane enough. @Moonchild and @athenian200 what say you? Once this checks out and is approved I will manually land this to trunk following our commit form which will be the example to follow for you @OlCe1 in the future. So is that ALL that is required? ---- Compare reference: https://repo.palemoon.org/MoonchildProductions/UXP/compare/master...OlCe1:FreeBSD-support
mattatobin added the
OS: Other
label пре 5 месеци
mattatobin added the
C: Memory
label пре 5 месеци
mattatobin added the
C: Build System
label пре 5 месеци
mattatobin added the
High Risk
label пре 5 месеци
mattatobin changed title from FreeBSD support to Update FreeBSD support пре 5 месеци
OlCe1 коментирира пре 5 месеци
Аутор

Which format error are you talking about? You mean “%zu” instead of “%u” when printing size_t stats?

The change is not “huge”, actually the gist of it is quite simple (the devil might be in the details, though).

More importantly, it does concern FreeBSD only. All other platforms are unaffected (even the introduction of ‘volatile’ for ‘malloc_initialized’ is a no-op for all platforms).

The only part where specific code is not as separated as I would have wanted is the addition of a condition in malloc_init_hard’s ifdefs and the introduction of “goto error” after ‘malloc_initializing’ has been set. It may be cleaner to use inline functions with conditional content to operate on the init lock. At least, I didn’t introduce new ifdefs (except the last one in this function), I just leveraged Windows’ ones.

Also, if you think there are too many ifdefs in mutex wrapping code, I can separate Linux and FreeBSD code instead.

Let me know if some things need refactoring.

Which format error are you talking about? You mean "%zu" instead of "%u" when printing size_t stats? The change is not "huge", actually the gist of it is quite simple (the devil might be in the details, though). More importantly, it does concern FreeBSD only. All other platforms are unaffected (even the introduction of 'volatile' for 'malloc_initialized' is a no-op for all platforms). The only part where specific code is not as separated as I would have wanted is the addition of a condition in malloc_init_hard's ifdefs and the introduction of "goto error" after 'malloc_initializing' has been set. It may be cleaner to use inline functions with conditional content to operate on the init lock. At least, I didn't introduce new ifdefs (except the last one in this function), I just leveraged Windows' ones. Also, if you think there are too many ifdefs in mutex wrapping code, I can separate Linux and FreeBSD code instead. Let me know if some things need refactoring.
OlCe1 коментирира пре 5 месеци
Аутор

Force-pushed the change ‘%u’ => ‘%zu’.

Force-pushed the change '%u' => '%zu'.
mattatobin коментирира пре 5 месеци
Коаутор

It was just one of the preprocessor directives. An errant space

It was just one of the preprocessor directives. An errant space
OlCe1 коментирира пре 5 месеци
Аутор

I looked again but don’t see any. May be in the penultimate commit, but I don’t think there is any in the last one.

I looked again but don't see any. May be in the penultimate commit, but I don't think there is any in the last one.
OlCe1 коментирира пре 5 месеци
Аутор

Pushed a new commit on top of the existing ones with simplifications.

  1. Removed the ‘malloc_initializing’ flag I introduced earlier, which required a small change in ‘malloc_mutex_init’ (see commit message).
  2. Do not hook on Linux code for mutexes. Keep FreeBSD case separate.

This reduces the diff and makes the code easier to read, which should help the review.

Pushed a new commit on top of the existing ones with simplifications. 1. Removed the 'malloc_initializing' flag I introduced earlier, which required a small change in 'malloc_mutex_init' (see commit message). 2. Do not hook on Linux code for mutexes. Keep FreeBSD case separate. This reduces the diff and makes the code easier to read, which should help the review.
OlCe1 коментирира пре 5 месеци
Аутор

So is that ALL that is required?

Yes, this is all there is to it.

> So is that ALL that is required? Yes, this is all there is to it.
Moonchild коментирира пре 5 месеци
Власник

I’m not going to have the opportunity around the holidays to review this properly. Maybe I can have a look between Chrismas and New Year, but no promises.

I'm not going to have the opportunity around the holidays to review this properly. Maybe I can have a look between Chrismas and New Year, but no promises.
OlCe1 коментирира пре 5 месеци
Аутор

No problem. The difficult details are normally all explained in the big comment section, but don’t hesitate to ask if something is unclear.

No problem. The difficult details are normally all explained in the big comment section, but don't hesitate to ask if something is unclear.
Moonchild коментирира пре 5 месеци
Власник

Ok I have had some time to look at this and overall looks OK but I need to make it a proper review so please open a pull request that I can review with code comments.

Ok I have had some time to look at this and overall looks OK but I need to make it a proper review so please open a pull request that I can review with code comments.
mattatobin коментирира пре 5 месеци
Коаутор

So I guess that will be me so that the patches are conformed properly. Ok.

@OlCe1 you don’t have to do anything except watch and learn.

So I guess that will be me so that the patches are conformed properly. Ok. @OlCe1 you don't have to do anything except watch and learn.
mattatobin added a new dependency пре 5 месеци
Moonchild removed a dependency пре 5 месеци
Moonchild added a new dependency пре 5 месеци
OlCe1 коментирира пре 5 месеци
Аутор

New PR at #1706.

New PR at https://repo.palemoon.org/MoonchildProductions/UXP/pulls/1706.
Moonchild added this to the (deleted) milestone пре 5 месеци
Moonchild modified the milestone from (deleted) to 29.0.0 пре 4 месеци
Пријавите се да се прикључе у овом разговору.
Нема фазе
No Assignees
3 учесника
Notifications
Due Date

No due date set.

Depends on
#1703 Add Modern FreeBSD Support
MoonchildProductions/UXP
Loading…
Још нема садржаја.