[
https://issues.apache.org/jira/browse/ARROW-1629?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16187512#comment-16187512
]
ASF GitHub Bot commented on ARROW-1629:
---------------------------------------
GitHub user wesm opened a pull request:
https://github.com/apache/arrow/pull/1151
ARROW-1629: [C++] Add miscellaneous DCHECKs and minor changes based on
infer tool output
This was an interesting journey through some esoterica. I went through all
the warnings/errors that infer (fbinfer.com) outputs and made changes if it
seemed warranted. Some of the checks might be overkill.
See https://gist.github.com/wesm/fc6809e4f4aaef3ecfeb21b8123627bc for a
summary of actions on each warning
Most of the errors that Infer wasn't happy about were already addressed by
DCHECKs. This was useful to go through all these cases -- in nearly all cases
the null references are impossible or would be the result of an error on behalf
of the application programmer. For example: we do not do array boundschecking
in most cases in production builds, but these boundschecks are included in
debug builds to assist with catching bugs caused by improper use by application
developers.
As a matter of convention, we should not use error `Status` to do parameter
validation or asserting pre-conditions that are the responsibility of the
library user. If parameter validation is required in binding code (e.g.
Python), then this validation should happen in the binding layer, not in the
core C++ library.
There are some other cases where we have a `std::shared_ptr<T>` out
variable with code like:
```
RETURN_NOT_OK(Foo(..., &out));
out->Method(...);
```
Here, infer complains that `out` could contain a null pointer, but our
contract with developers is that if `Foo` returns successfully that `out` is
non-null.
Interestingly, infer doesn't like some stack variables that are bound in
C++11 lambda expressions. I noted these in the gist with `LAMBDA`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wesm/arrow fix-infer-issues
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/arrow/pull/1151.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1151
----
commit 05316ce48a39067e64ef23d22fc3ef748029e251
Author: Wes McKinney <[email protected]>
Date: 2017-09-30T20:55:19Z
Fix miscellaneous things that infer does not like. Make some Python helper
functions internal / non-exported
Change-Id: I4b91ecce99dde4e87dcf70f026df1af17d2f067f
commit 5ff3e3a5ed6c72c044d86bc9c5215edb8ef4d847
Author: Wes McKinney <[email protected]>
Date: 2017-09-30T20:58:07Z
Compilation fix
Change-Id: I2ef789a21cdcaf1573480841af1ac3c45e747133
commit 75131a6b91c4568fababfa310db9412c77114330
Author: Wes McKinney <[email protected]>
Date: 2017-09-30T22:42:07Z
Some more minor infer fixes
Change-Id: I75c624b0ca1f0daf3940c4cd0c4ec5497a04285f
commit 22c5d361607ad82e9951e8ccb013f1c1c81cdfa1
Author: Wes McKinney <[email protected]>
Date: 2017-10-01T00:46:36Z
Address a couple more infer warnings
Change-Id: Ib30ff3d4784983342eca31405b8916dd4474be03
commit 5aa86ce2373d67c17a7859b1c6405a9ca226e052
Author: Wes McKinney <[email protected]>
Date: 2017-10-01T18:54:15Z
More DCHECK esoterica / tweaks based on infer report
Change-Id: I4e01f340e12d9aa6d319aa77dc05b9bfe6bfa970
----
> [C++] Fix problematic code paths identified by infer tool
> ---------------------------------------------------------
>
> Key: ARROW-1629
> URL: https://issues.apache.org/jira/browse/ARROW-1629
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++
> Reporter: Wes McKinney
> Assignee: Wes McKinney
> Labels: pull-request-available
> Fix For: 0.8.0
>
>
> I'm making a pass over the output from ARROW-1626
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
