Closed Bug 1412221 Opened 3 years ago Closed 3 years ago

Miscellaneous code cleanups in mozjemalloc.cpp

Categories

(Core :: Memory Allocator, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

No description provided.
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
(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.
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 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 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 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 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 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+
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 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
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.