#1699 Update FreeBSD support

Fermé
créé il y a 5 mois par OlCe1 · 12 commentaires
OlCe1 a commenté il y a 5 mois

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 a commenté il y a 5 mois
Collaborateur

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 a ajouté l’étiquette
OS: Other
il y a 5 mois
mattatobin a ajouté l’étiquette
C: Memory
il y a 5 mois
mattatobin a ajouté l’étiquette
C: Build System
il y a 5 mois
mattatobin a ajouté l’étiquette
High Risk
il y a 5 mois
mattatobin a modifié le titre de FreeBSD support à Update FreeBSD support il y a 5 mois
OlCe1 a commenté il y a 5 mois
Publier

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 a commenté il y a 5 mois
Publier

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

Force-pushed the change '%u' => '%zu'.
mattatobin a commenté il y a 5 mois
Collaborateur

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

It was just one of the preprocessor directives. An errant space
OlCe1 a commenté il y a 5 mois
Publier

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 a commenté il y a 5 mois
Publier

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 a commenté il y a 5 mois
Publier

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 a commenté il y a 5 mois
Propriétaire

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 a commenté il y a 5 mois
Publier

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 a commenté il y a 5 mois
Propriétaire

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 a commenté il y a 5 mois
Collaborateur

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 a ajouté une nouvelle dépendance il y a 5 mois
Moonchild a supprimé une dépendance il y a 5 mois
Moonchild a ajouté une nouvelle dépendance il y a 5 mois
OlCe1 a commenté il y a 5 mois
Publier

New PR at #1706.

New PR at https://repo.palemoon.org/MoonchildProductions/UXP/pulls/1706.
Moonchild a fermé ce ticket il y a 5 mois
Moonchild a ajouté cela au jalon (supprimée) il y a 5 mois
Moonchild a modifié le jalon de (supprimée) à 29.0.0 il y a 4 mois
Connectez-vous pour rejoindre cette conversation.
Aucun jalon
Pas d'assignataires
3 participants
Notifications
Échéance

Aucune échéance n'a été définie.

Dépend de
#1703 Add Modern FreeBSD Support
MoonchildProductions/UXP
Chargement…
Il n'existe pas encore de contenu.