Avoid using non-const function params in a few places

[xunjieli]

Non-const function params are not allowed in Google cpp style guide, see below: https://google.github.io/styleguide/cppguide.html "In fact it is a very strong convention in Google code that input arguments are values or const references while output arguments are pointers. Input parameters may be const pointers, but we never allow non-const reference parameters except when required by convention, e.g., swap()."

This fixes a few places to use non-const pointers instead of non-const refs as function out params.

[mlin]

@xunjieli can you elaborate on what problem / pain point is relieved by this change?

I'm not sure about the style guide in the case of a smart pointer object. Passing a pointer to a smart pointer admits the state of "null pointer to a smart pointer," distinct from "nonnull pointer to a null smart pointer" which seems rather confusing to me. I'm open to it if there's a specific way it helps you, though.

Looking it over again, the reference-counted shared_ptr is probably not necessary for dataset_header. It'd be impractical for many other reasons to process so many samples in one process that we'd need to page the headers in and out of memory. We could all scope all the header objects to the lifetime of the BCFData object and just pass around const bcf_hdr_t*. OTOH the current over-engineered state isn't too harmful.

[xunjieli]

@mlin I have an implementation of BCFData that caches states (opened vcfFile, tbx_t and bcf_hdr_t) directly from a Cloud-hosted gVCF file. The pain point is that the function signatures of BCFData is not Google code style compliant (because of the use of non-const refs as function params), so I am having quite some difficulty in getting my BCFData subclass through our internal code review process.

I am not sure if changing std::shared_ptr<const bcf_hdr_t*>& to const bcf_hdr_t* will work -- If the hdr is scoped to the lifetime of BCFData, the raw const pointer returned to the caller may or may not be valid, since the cache states can be invalidated without the knowledge of the caller (e.g. by an LRU cache owned by the BCFData).

[mlin]

It's a bit of a thin reason, but we'll go with it :)

In the Travis build, it like two other classes BCFKeyValueData and a unit-testing harness VCFData need updating to match the new superclass signature. Do you want to hack on those or shall I? either way is fine, just want to make sure we don't duplicate effort.

[xunjieli]

Thanks @mlin for the catch. I've modified test/utils.cc

[coveralls]

Coverage remained the same at 93.381% when pulling d77c143a3b5c1276b1578d2c86e2490aae507fdd on xunjieli:non-const-ref-param into a4271d95e6a7246c53b011e4f0b0c8f9e62412cf on dnanexus-rnd:master.