#1699 Update FreeBSD support

Zamknięty
otworzone 5 miesięcy temu przez OlCe1 · 12 komentarzy
OlCe1 skomentował(-a) 5 miesięcy temu

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 skomentował(-a) 5 miesięcy temu
Współpracownik

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 dodał(-ęła) etykietę
OS: Other
5 miesięcy temu
mattatobin dodał(-ęła) etykietę
C: Memory
5 miesięcy temu
mattatobin dodał(-ęła) etykietę
C: Build System
5 miesięcy temu
mattatobin dodał(-ęła) etykietę
High Risk
5 miesięcy temu
mattatobin zmieniono tytuł z FreeBSD support na Update FreeBSD support 5 miesięcy temu
OlCe1 skomentował(-a) 5 miesięcy temu
Autor

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 skomentował(-a) 5 miesięcy temu
Autor

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

Force-pushed the change '%u' => '%zu'.
mattatobin skomentował(-a) 5 miesięcy temu
Współpracownik

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

It was just one of the preprocessor directives. An errant space
OlCe1 skomentował(-a) 5 miesięcy temu
Autor

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 skomentował(-a) 5 miesięcy temu
Autor

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 skomentował(-a) 5 miesięcy temu
Autor

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 skomentował(-a) 5 miesięcy temu
Właściciel

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 skomentował(-a) 5 miesięcy temu
Autor

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 skomentował(-a) 5 miesięcy temu
Właściciel

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 skomentował(-a) 5 miesięcy temu
Współpracownik

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 dodał nową zależność 5 miesięcy temu
Moonchild usunął zależność 5 miesięcy temu
Moonchild dodał nową zależność 5 miesięcy temu
OlCe1 skomentował(-a) 5 miesięcy temu
Autor

New PR at #1706.

New PR at https://repo.palemoon.org/MoonchildProductions/UXP/pulls/1706.
Moonchild zamknął(-ęła) to zgłoszenie 5 miesięcy temu
Moonchild dodaje to do kamienia milowego (usunięto) 5 miesięcy temu
Moonchild zmienia kamień milowy z (usunięto) na 29.0.0 4 miesięcy temu
Zaloguj się, aby dołączyć do tej rozmowy.
Brak kamienia milowego
Brak przypisanych
Uczestnicy 3
Powiadomienia
Termin realizacji

Brak ustawionego terminu realizacji.

Zależy od
#1703 Add Modern FreeBSD Support
MoonchildProductions/UXP
Ładowanie…
Nie ma jeszcze treści.