Miscellaneous code cleanups in mozjemalloc.cpp

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks 2 bugs)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

2 years ago
No description provided.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8922683 [details]
Bug 1412221 - Use pages_unmap instead of repeating the same code.

https://reviewboard.mozilla.org/r/193826/#review198922


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: memory/build/mozjemalloc.cpp:1575
(Diff revision 1)
> +pages_unmap(void *addr, size_t size)
> +{
> +	if (munmap(addr, size) == -1) {
> +		char buf[STRERROR_BUF];
> +
> +		if (strerror_r(errno, buf, sizeof(buf)) == 0) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

                if (strerror_r(errno, buf, sizeof(buf)) == 0) {
                                                           ^
                                                           nullptr
Assignee

Comment 7

2 years ago
(In reply to Static Analysis Bot [:clangbot] from comment #6)
> Warning: Use nullptr [clang-tidy: modernize-use-nullptr]
> 
>                 if (strerror_r(errno, buf, sizeof(buf)) == 0) {
>                                                            ^
>                                                            nullptr

Aha: bug #1412214.
Assignee

Comment 8

2 years ago
Note the clang-format patch is massive, but only covers ~20% of the file size. Which tells how much had been rewritten/reformatted already.

Comment 9

2 years ago
mozreview-review
Comment on attachment 8922681 [details]
Bug 1412221 - Fix clang-tidy warnings in mozjemalloc.cpp.

https://reviewboard.mozilla.org/r/193822/#review199264

::: memory/build/mozjemalloc.cpp:4509
(Diff revision 1)
>  
>  inline void*
>  BaseAllocator::calloc(size_t aNum, size_t aSize)
>  {
>    void *ret;
>    size_t num_size;

You could move these declarations down lower.
Attachment #8922681 - Flags: review?(n.nethercote) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8922683 [details]
Bug 1412221 - Use pages_unmap instead of repeating the same code.

https://reviewboard.mozilla.org/r/193826/#review199268

::: memory/build/mozjemalloc.cpp:1570
(Diff revision 1)
>  	}
>  }
>  #else
>  
> +static void
> +pages_unmap(void *addr, size_t size)

Rename the parameter at the same time?
Attachment #8922683 - Flags: review?(n.nethercote) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8922684 [details]
Bug 1412221 - Change comments to use C++ style // instead of /* */.

https://reviewboard.mozilla.org/r/193828/#review199270

Yay! I would give you two r+'s for this if I could.
Attachment #8922684 - Flags: review?(n.nethercote) → review+

Comment 12

2 years ago
mozreview-review
Comment on attachment 8922685 [details]
Bug 1412221 - Run clang-format on mozjemalloc.cpp.

https://reviewboard.mozilla.org/r/193830/#review199272

::: memory/build/mozjemalloc.cpp:2341
(Diff revision 1)
> -		    0, 1, 0, 2, 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 4,
> -		    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5,
> -		    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> -		    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6,
> -		    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> -		    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +      0, 1, 0, 2, 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0,
> +      0, 0, 0, 0, 0, 0, 0, 0, 0, 5, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +      0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, 0, 0,
> +      0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +      0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +      0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7

The old table had rows of 16, which seems better than the new 22. Seems like a good candidate for `// clang-format off` and `// clang-format on`.
Attachment #8922685 - Flags: review?(n.nethercote) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8922682 [details]
Bug 1412221 - Remove unnecessary parentheses around return values.

https://reviewboard.mozilla.org/r/193824/#review199266

Finally! :)  This was such a weird jemalloc-ism.
Attachment #8922682 - Flags: review?(n.nethercote) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

2 years ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/58070c2511c6
Fix clang-tidy warnings in mozjemalloc.cpp. r=njn
https://hg.mozilla.org/integration/autoland/rev/e2b391ae4688
Remove unnecessary parentheses around return values. r=njn
https://hg.mozilla.org/integration/autoland/rev/eefc284bbf13
Use pages_unmap instead of repeating the same code. r=njn
https://hg.mozilla.org/integration/autoland/rev/6278aa5fec1d
Change comments to use C++ style // instead of /* */. r=njn
https://hg.mozilla.org/integration/autoland/rev/f75f427fde1d
Run clang-format on mozjemalloc.cpp. r=njn

Comment 20

2 years ago
mozreview-review
Comment on attachment 8922683 [details]
Bug 1412221 - Use pages_unmap instead of repeating the same code.

https://reviewboard.mozilla.org/r/193826/#review199288


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: memory/build/mozjemalloc.cpp:1575
(Diff revision 2)
> +pages_unmap(void *aAddr, size_t aSize)
> +{
> +	if (munmap(aAddr, aSize) == -1) {
> +		char buf[STRERROR_BUF];
> +
> +		if (strerror_r(errno, buf, sizeof(buf)) == 0) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

                if (strerror_r(errno, buf, sizeof(buf)) == 0) {
                                                           ^
                                                           nullptr

Comment 21

2 years ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/655626cb97ca
Backed out changeset 5 changesets for build bustage at memory/build/mozjemalloc.cpp:4390: jump to label 'RETURN'. r=backout
https://hg.mozilla.org/integration/autoland/rev/58a28d2b7d98
Fix clang-tidy warnings in mozjemalloc.cpp. r=njn
https://hg.mozilla.org/integration/autoland/rev/4cc4bc2ccd09
Remove unnecessary parentheses around return values. r=njn
https://hg.mozilla.org/integration/autoland/rev/cc9894200012
Use pages_unmap instead of repeating the same code. r=njn
https://hg.mozilla.org/integration/autoland/rev/a0b897b71450
Change comments to use C++ style // instead of /* */. r=njn
https://hg.mozilla.org/integration/autoland/rev/fe6a64b6246b
Run clang-format on mozjemalloc.cpp. r=njn
https://hg.mozilla.org/integration/autoland/rev/0acd42f92d6d
Fix build bustage in mozjemalloc.cpp. r=me
You need to log in before you can comment on or make changes to this bug.