Don't bother checking pointers before delete[] and FX_Free().

Don't bother checking pointers before delete[] and FX_Free().

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/cb62e7657b3a9a04142028a4e6614029a08e894b

[Lei Zhang, 2015-08-14 19:29:46]

thestig@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2015-08-14 19:43:46]

tsepez@chromium.org changed reviewers:
+ brucedawson@chromium.org

[Tom Sepez, 2015-08-14 19:44:29]

+bruce because he knows everything.

I thought in old C programs free(null) was not desirable; has something changed?

[brucedawson, 2015-08-14 20:12:26]

I think you are correct, twice. In old C programs free(null) was
undefined/illegal, but now it is defined to do nothing.

I don't have a copy of the C standard to check this, and I don't know when the
change occurred, but it was quite a while ago.

I would assume that if our memory checking tools allow free(null) then that is a
powerful sign that it is okay on all platforms, so checking in code that does
free(null) will either fail on one of the sanitizers or be perfectly fine. I'm
betting perfectly fine.

And, fewer checks equals smaller code equals goodness.

[Tom Sepez, 2015-08-14 21:21:34]

lgtm

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_font/...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_font/...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:304: FX_Free(m_pMapping);
nit: indentations.

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_font/...
File core/src/fpdfapi/fpdf_font/ttgsubtable.h (right):

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_font/...
core/src/fpdfapi/fpdf_font/ttgsubtable.h:103: delete[] LookupListIndex;
nit: indentation.  Several other places.  clang-format should fix it?

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_page/...
File core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp (right):

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:830: m_pDomains = NULL;
nit: Pointless NULL assignment(s).

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_page/...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (right):

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:962:
FX_Free((void*)m_pData);
nit: void cast not needed?

[Lei Zhang, 2015-08-14 21:27:51]

On 2015/08/14 19:44:29, Tom Sepez wrote:
> +bruce because he knows everything.
> 
> I thought in old C programs free(null) was not desirable; has something
changed?

"man 3 free" dated 2012 says "If ptr is NULL, no operation is performed."

[Tom Sepez, 2015-08-14 21:33:19]

On 2015/08/14 21:27:51, Lei Zhang wrote:
> On 2015/08/14 19:44:29, Tom Sepez wrote:
> > +bruce because he knows everything.
> > 
> > I thought in old C programs free(null) was not desirable; has something
> changed?
> 
> "man 3 free" dated 2012 says "If ptr is NULL, no operation is performed."
Yeah, but that's just linux.  What about the bizzarros?

[Lei Zhang, 2015-08-14 21:43:15]

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_font/...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_font/...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:304: FX_Free(m_pMapping);
On 2015/08/14 21:21:34, Tom Sepez wrote:
> nit: indentations.

I was being sloppy and hoping for clang-format to fix it all. Apparently not.

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_font/...
File core/src/fpdfapi/fpdf_font/ttgsubtable.h (right):

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_font/...
core/src/fpdfapi/fpdf_font/ttgsubtable.h:103: delete[] LookupListIndex;
On 2015/08/14 21:21:34, Tom Sepez wrote:
> nit: indentation.  Several other places.  clang-format should fix it?

I don't know why clang-format did all the ones above, but not this one.

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_page/...
File core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp (right):

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:830: m_pDomains = NULL;
On 2015/08/14 21:21:34, Tom Sepez wrote:
> nit: Pointless NULL assignment(s).

Done.

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_page/...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (right):

https://codereview.chromium.org/1297713003/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:962:
FX_Free((void*)m_pData);
On 2015/08/14 21:21:34, Tom Sepez wrote:
> nit: void cast not needed?

Done.

[brucedawson, 2015-08-14 21:51:22]

I meant to list my source for the claim that free(NULL) is legal. I searched for
"free(null)" and that led to several stackoverflow questions and a lot of the
answers quoted from the C standard to say that it was fine. This isn't perfect
science, but it matches what I learned about five years ago so I believe it is
true.

The stackoverflow pages do point out that man pages are not normative about the
C language, but say the C standard also guarantees this behavior.

[Lei Zhang, 2015-08-14 21:56:59]

On 2015/08/14 21:33:19, Tom Sepez wrote:
> On 2015/08/14 21:27:51, Lei Zhang wrote:
> > On 2015/08/14 19:44:29, Tom Sepez wrote:
> > > +bruce because he knows everything.
> > > 
> > > I thought in old C programs free(null) was not desirable; has something
> > changed?
> > 
> > "man 3 free" dated 2012 says "If ptr is NULL, no operation is performed."
> Yeah, but that's just linux.  What about the bizzarros?

The C99 standard http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf
section 7.20.3.2 and the C89 standard section 4.10.3.2 both say the same thing.
I don't think we have any bizzaros here.

[Tom Sepez, 2015-08-14 21:58:17]

> I don't think we have any bizzaros here.
Let's land it.

[Lei Zhang, 2015-08-14 22:45:44]

Committed patchset #3 (id:40001) manually as
cb62e7657b3a9a04142028a4e6614029a08e894b (presubmit successful).