[Asterisk-code-review] MALLOC DEBUG: Always use the API. (asterisk[master])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/8366 )

Change subject: MALLOC_DEBUG: Always use the API.
......................................................................


Patch Set 1:

(4 comments)

> > (1 comment)
 > >
 > > I have not done a complete review yet but if this eliminates the
 > > ABI difference then MALLOC_DEBUG should be added to the list of
 > > exceptions in build_tools/make_buildopts_h.
 > 
 > The intent is to eliminate the API differences between MALLOC_DEBUG
 > being enabled and it being disabled.  So yes I'll need to add it to
 > the exceptions.

Once the make_buildopts_h change is made I can +1.

https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/astmm.h
File include/asterisk/astmm.h:

https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/astmm.h@174
PS1, Line 174: #else
             : #error "NEVER INCLUDE astmm.h DIRECTLY!!"
Nit: this can be removed as it will be harmless to directly include astmm.h.  Feel free to ignore this comment if you wish.


https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/hashtab.h
File include/asterisk/hashtab.h:

https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/hashtab.h@261
PS1, Line 261: #define ast_hashtab_create(initial_buckets, compare, resize, newsize, hash, do_locking) \
Thank you!  Really hate when macro's have generic a,b,c,d argument names, makes IDE tool-tips useless.


https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/stringfields.h
File include/asterisk/stringfields.h:

https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/stringfields.h@231
PS1, Line 231: 	/* v-- MALLOC_DEBUG information */
This is unfortunate.  I know it's just 2 pointers and an int but we use string fields in a lot of structures.

Alternative: What if we updated __ast_string_field_alloc_space, __ast_string_field_ptr_build and __ast_string_field_ptr_build_va to use the callers file/line/func?  Be using the ast_string_field_mgr owner_* we're actually obscuring situations where we allocate additional space.  If we could see which locations cause additional space to be allocated this might allow us to make minor changes to optimize memory usage (like maybe the original allocation size should exclude fields that will always be rewritten).

Feel free to defer or even refuse this comment.


https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/utils.h
File include/asterisk/utils.h:

https://gerrit.asterisk.org/#/c/8366/1/include/asterisk/utils.h@536
PS1, Line 536: #define ast_free free
> You did notice the comment about 10 lines above right?
I see now. please disregard my previous comments at this location.



-- 
To view, visit https://gerrit.asterisk.org/8366
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic07ad80b2c2df894db984cf27b16a69383ce0e10
Gerrit-Change-Number: 8366
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 22 Feb 2018 19:12:47 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180222/1cf61d6b/attachment.html>