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.

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?

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.

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`.

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.

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

https://hg.mozilla.org/mozilla-central/rev/58a28d2b7d98 https://hg.mozilla.org/mozilla-central/rev/4cc4bc2ccd09 https://hg.mozilla.org/mozilla-central/rev/cc9894200012 https://hg.mozilla.org/mozilla-central/rev/a0b897b71450 https://hg.mozilla.org/mozilla-central/rev/fe6a64b6246b https://hg.mozilla.org/mozilla-central/rev/0acd42f92d6d