#1699 Update FreeBSD support

已關閉
OlCe15 月之前建立 · 12 條評論

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 closed this issue 5 月之前
Moonchild 新增至(已刪除) 里程碑 5 月之前
Moonchild 4 月之前 修改了里程碑 (已刪除)29.0.0
登入 才能加入這對話。
未選擇里程碑
No Assignees
3 參與者
訊息
Due Date

No due date set.

Depends on
#1703 Add Modern FreeBSD Support
MoonchildProductions/UXP
Loading…
尚未有任何內容