Get rid of most instance of 'foo == NULL'

Get rid of most instance of 'foo == NULL'

R=tsepez@chromium.org

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

[Lei Zhang, 2015-12-12 02:31:43]

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

[Lei Zhang, 2015-12-12 02:31:44]

And after foo != NULL, do bar == NULL.

https://codereview.chromium.org/1520063002/diff/1/core/include/fxcrt/fx_basic.h
File core/include/fxcrt/fx_basic.h (right):

https://codereview.chromium.org/1520063002/diff/1/core/include/fxcrt/fx_basic...
core/include/fxcrt/fx_basic.h:897: FX_BOOL NotNull() const { return !IsNull(); }
or leave it as is?

https://codereview.chromium.org/1520063002/diff/1/core/src/fpdfdoc/doc_utils.cpp
File core/src/fpdfdoc/doc_utils.cpp (left):

https://codereview.chromium.org/1520063002/diff/1/core/src/fpdfdoc/doc_utils....
core/src/fpdfdoc/doc_utils.cpp:232: if (pFormDict == NULL) {
Can't fail. Need to make another pass through all the CPDF_Object::Create()
calls and convert them to new CPDF_Object.

https://codereview.chromium.org/1520063002/diff/1/core/src/fxge/agg/src/fx_ag...
File core/src/fxge/agg/src/fx_agg_driver.cpp (right):

https://codereview.chromium.org/1520063002/diff/1/core/src/fxge/agg/src/fx_ag...
core/src/fxge/agg/src/fx_agg_driver.cpp:559: if (m_pDevice->GetPalette()) {
There's a bunch of places where I flipped the logic.

https://codereview.chromium.org/1520063002/diff/1/core/src/fxge/dib/fx_dib_co...
File core/src/fxge/dib/fx_dib_composite.cpp (right):

https://codereview.chromium.org/1520063002/diff/1/core/src/fxge/dib/fx_dib_co...
core/src/fxge/dib/fx_dib_composite.cpp:963: if (dest_alpha_scan) {
Another flip, but the two blocks are very similar.

[Tom Sepez, 2015-12-14 18:27:01]

Great.  I was planning on doing this recently, too.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page.cpp:723: return pos ?
(CPDF_PageObject*)m_ObjectList.GetAt(pos) : nullptr;
nit: static_cast as long as you are re-writing it.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:1534: if (m_pCS != other.m_pCS
|| !m_pCS) {
nit: maybe a boolean expression (doublecheck my logic) instead of an if,

return m_pCS && m_pCS == other.m_pCS && FXSYS_memcmp(...) == 0;

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_document.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_document.cpp:326: return m_pParser ?
m_pParser->IsOwner() : TRUE;
nit: return !m_pParser || m_pParser->IsOwner();

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1484: return
m_pSecurityHandler ? m_pSecurityHandler->IsOwner() : TRUE;
nit: return !m_pSecurityHandler || m_pSecurityHandler->IsOwner();
(can you tell I don't like TRUE or FALSE appearing in a ? operator)

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_ac...
File core/src/fpdfdoc/doc_action.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_ac...
core/src/fpdfdoc/doc_action.cpp:256: return FALSE;
nit: return m_pDict && m_pDictKeyExist(...);

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_ap...
File core/src/fpdfdoc/doc_ap.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_ap...
core/src/fpdfdoc/doc_ap.cpp:423: if (!pAPDict) {
nit: can this fail? I though I fixed these.  hmmm.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:584: return FALSE;
nit: return pField && !csNewFieldName.IsEmpty() && ValidateFieldName(...);

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:709: return m_pFormDict ?
m_pFormDict->GetBoolean("NeedAppearances") : FALSE;
nit: return m_pFormDict && m_pFormDict->GetBoolean(...);

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:820: if (!m_pFormDict) {
nit: return m_pFormDict ? ... : 0;

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_formcontrol.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_formcontrol.cpp:321: return
m_pField->m_pForm->GetFormAlignment();
nit: maybe flip this around so as to lose the !

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text_search.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_search.cpp:306: if (!pObject) {
nit: or just

 if (pObjext && pObject->m_Type == PDFPAGE_TEXT)
    textstream.ProceessObjext(...);

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_jpeg.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpeg.cpp:536: return FALSE;
nit: This is making my head hurt.  If its less than 8, we come in here and the
first part of the assert is false, and we fault.  If there is a pallette, we
come in here, and the second part of the assert is false, and we fault. Either
way we fault.  Maybe remove it.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_array.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_array.cpp:163: if (!pIndex[i]) {
nit: flip this around and lose the !

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_list.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_list.cpp:119: CNode* pNode = (CNode*)startAfter;
nit: This calls for a ? operator here.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_wstring.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_wstring.cpp:281: if (!m_pData) {
nit: maybe flip around to avoid !

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxge/dib/fx_di...
File core/src/fxge/dib/fx_dib_engine.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxge/dib/fx_di...
core/src/fxge/dib/fx_dib_engine.cpp:852: return m_pStretchEngine ?
m_pStretchEngine->Continue(pPause) : FALSE;
nit: convert to && to get rid of FALSE in ? Oper.

https://codereview.chromium.org/1520063002/diff/20001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/Document.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/Document.cpp:1266: m_pHead = m_pEnd = pNewIcon;
Nit: one per line so long as you're re-writing this.

[Lei Zhang, 2015-12-15 01:58:36]

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page.cpp:723: return pos ?
(CPDF_PageObject*)m_ObjectList.GetAt(pos) : nullptr;
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: static_cast as long as you are re-writing it.

Still fits on 1 line, phew.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:1534: if (m_pCS != other.m_pCS
|| !m_pCS) {
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: maybe a boolean expression (doublecheck my logic) instead of an if,
> 
> return m_pCS && m_pCS == other.m_pCS && FXSYS_memcmp(...) == 0;

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_document.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_document.cpp:326: return m_pParser ?
m_pParser->IsOwner() : TRUE;
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: return !m_pParser || m_pParser->IsOwner();

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1484: return
m_pSecurityHandler ? m_pSecurityHandler->IsOwner() : TRUE;
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: return !m_pSecurityHandler || m_pSecurityHandler->IsOwner();
> (can you tell I don't like TRUE or FALSE appearing in a ? operator)

Mmmmhmm

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_ac...
File core/src/fpdfdoc/doc_action.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_ac...
core/src/fpdfdoc/doc_action.cpp:256: return FALSE;
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: return m_pDict && m_pDictKeyExist(...);

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_ap...
File core/src/fpdfdoc/doc_ap.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_ap...
core/src/fpdfdoc/doc_ap.cpp:423: if (!pAPDict) {
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: can this fail? I though I fixed these.  hmmm.

I thought I fixed these too.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:584: return FALSE;
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: return pField && !csNewFieldName.IsEmpty() && ValidateFieldName(...);

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:709: return m_pFormDict ?
m_pFormDict->GetBoolean("NeedAppearances") : FALSE;
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: return m_pFormDict && m_pFormDict->GetBoolean(...);

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:820: if (!m_pFormDict) {
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: return m_pFormDict ? ... : 0;

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_formcontrol.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_formcontrol.cpp:321: return
m_pField->m_pForm->GetFormAlignment();
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: maybe flip this around so as to lose the !

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text_search.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_search.cpp:306: if (!pObject) {
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: or just
> 
>  if (pObjext && pObject->m_Type == PDFPAGE_TEXT)
>     textstream.ProceessObjext(...);

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_jpeg.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpeg.cpp:536: return FALSE;
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: This is making my head hurt.  If its less than 8, we come in here and the
> first part of the assert is false, and we fault.  If there is a pallette, we
> come in here, and the second part of the assert is false, and we fault. Either
> way we fault.  Maybe remove it.

Right, this is: if (foo) { ASSERT(!foo); } -> makes no sense -> removed.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_array.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_array.cpp:163: if (!pIndex[i]) {
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: flip this around and lose the !

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_list.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_list.cpp:119: CNode* pNode = (CNode*)startAfter;
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: This calls for a ? operator here.

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_wstring.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_wstring.cpp:281: if (!m_pData) {
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: maybe flip around to avoid !

Done.

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxge/dib/fx_di...
File core/src/fxge/dib/fx_dib_engine.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/core/src/fxge/dib/fx_di...
core/src/fxge/dib/fx_dib_engine.cpp:852: return m_pStretchEngine ?
m_pStretchEngine->Continue(pPause) : FALSE;
On 2015/12/14 18:27:00, Tom Sepez wrote:
> nit: convert to && to get rid of FALSE in ? Oper.

Done.

https://codereview.chromium.org/1520063002/diff/20001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/Document.cpp (right):

https://codereview.chromium.org/1520063002/diff/20001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/Document.cpp:1266: m_pHead = m_pEnd = pNewIcon;
On 2015/12/14 18:27:00, Tom Sepez wrote:
> Nit: one per line so long as you're re-writing this.

Done.

[Tom Sepez, 2015-12-15 02:06:09]

lgtm

[Lei Zhang, 2015-12-15 02:29:31]

Description was changed from

==========
Get rid of most instance of 'foo == NULL'
==========

to

==========
Get rid of most instance of 'foo == NULL'

R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/e385244f8cd6ae376f6b3cf1265a0795d5d3...
==========

[Lei Zhang, 2015-12-15 02:29:33]

Committed patchset #5 (id:80001) manually as
e385244f8cd6ae376f6b3cf1265a0795d5d30eff (presubmit successful).