[libcamera-devel] [PATCH 2/2] libcamera: Use 'struct' for structure types

[Jacopo Mondi]

Hi Niklas,

On Wed, Jan 02, 2019 at 11:54:00PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2019-01-02 13:02:56 +0100, Jacopo Mondi wrote:
> > Add back the 'struct' keyword for structure types.
> > C++ allows omitting the 'struct' keywork. Add it back to make clear
> > we're dealing with structures and not class types.
> >
> > While at there re-align lines to first open brace, or angular brace for
> > casts, and re-sort lines to have the longest one first in populate()
> > function.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/media_device.cpp | 33 ++++++++++++++++-----------------
> >  1 file changed, 16 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> > index 34206c8..715d971 100644
> > --- a/src/libcamera/media_device.cpp
> > +++ b/src/libcamera/media_device.cpp
> > @@ -205,10 +205,10 @@ void MediaDevice::close()
> >  int MediaDevice::populate()
> >  {
> >  	struct media_v2_topology topology = { };
> > +	struct media_v2_interface *interfaces = nullptr;
> >  	struct media_v2_entity *ents = nullptr;
> >  	struct media_v2_link *links = nullptr;
> >  	struct media_v2_pad *pads = nullptr;
> > -	struct media_v2_interface *interfaces = nullptr;
> >  	__u64 version = -1;
> >  	int ret;
>
> I don't like this. I think the variables should be declared and
> processed in the same order as in struct media_v2_topology. It makes the
> code so much more readable when comparing it with the kernel header.
>
> - ptr_entities
> - ptr_interfaces
> - ptr_pads
> - ptr_links
>
> I agree reverse xmas tree is usually the way to sort this but as this is
> references to a structure IMHO it takes precedence.
>

Thanks, I see. I'll leave it out then.
From now that we have stabilized a little more on a style, I'll waste less
time on this minor nits.

> >
> > @@ -219,10 +219,10 @@ int MediaDevice::populate()
> >  	 */
> >  	while (true) {
> >  		topology.topology_version = 0;
> > +		topology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);
> >  		topology.ptr_entities = reinterpret_cast<__u64>(ents);
> >  		topology.ptr_links = reinterpret_cast<__u64>(links);
> >  		topology.ptr_pads = reinterpret_cast<__u64>(pads);
> > -		topology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);
> >
> >  		ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> >  		if (ret < 0) {
> > @@ -235,15 +235,15 @@ int MediaDevice::populate()
> >  		if (version == topology.topology_version)
> >  			break;
> >
> > +		delete[] interfaces;
> >  		delete[] links;
> >  		delete[] ents;
> >  		delete[] pads;
> > -		delete[] interfaces;
> >
> > -		ents = new media_v2_entity[topology.num_entities];
> > -		links = new media_v2_link[topology.num_links];
> > -		pads = new media_v2_pad[topology.num_pads];
> > -		interfaces = new media_v2_interface[topology.num_interfaces];
> > +		interfaces = new struct media_v2_interface[topology.num_interfaces];
> > +		ents = new struct media_v2_entity[topology.num_entities];
> > +		links = new struct media_v2_link[topology.num_links];
> > +		pads = new struct media_v2_pad[topology.num_pads];
> >
> >  		version = topology.topology_version;
> >  	}
> > @@ -254,10 +254,10 @@ int MediaDevice::populate()
> >  	    populateLinks(topology))
> >  		valid_ = true;
> >
> > +	delete[] interfaces;
> >  	delete[] links;
> >  	delete[] ents;
> >  	delete[] pads;
> > -	delete[] interfaces;
> >
> >  	if (!valid_) {
> >  		clear();
> > @@ -417,17 +417,16 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo
> >   */
> >  bool MediaDevice::populateEntities(const struct media_v2_topology &topology)
> >  {
> > -	media_v2_entity *mediaEntities = reinterpret_cast<media_v2_entity *>
> > -					 (topology.ptr_entities);
> > +	struct media_v2_entity *mediaEntities = reinterpret_cast<struct media_v2_entity *>
> > +								(topology.ptr_entities);
> >
> >  	for (unsigned int i = 0; i < topology.num_entities; ++i) {
> >  		/*
> >  		 * Find the interface linked to this entity to get the device
> >  		 * node major and minor numbers.
> >  		 */
> > -		struct media_v2_interface *iface =
> > -			findInterface(topology, mediaEntities[i].id);
> > -
> > +		struct media_v2_interface *iface = findInterface(topology,
> > +								 mediaEntities[i].id);
>
> Nit-pic, I find the first way of writing this much more readable.

Ok, as you wish.
I'll resubmit (and push?) only the addition of 'struct'

Thanks
  j

>
> >  		MediaEntity *entity;
> >  		if (iface)
> >  			entity = new MediaEntity(&mediaEntities[i],
> > @@ -449,8 +448,8 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)
> >
> >  bool MediaDevice::populatePads(const struct media_v2_topology &topology)
> >  {
> > -	media_v2_pad *mediaPads = reinterpret_cast<media_v2_pad *>
> > -				  (topology.ptr_pads);
> > +	struct media_v2_pad *mediaPads = reinterpret_cast<struct media_v2_pad *>
> > +							 (topology.ptr_pads);
> >
> >  	for (unsigned int i = 0; i < topology.num_pads; ++i) {
> >  		unsigned int entity_id = mediaPads[i].entity_id;
> > @@ -478,8 +477,8 @@ bool MediaDevice::populatePads(const struct media_v2_topology &topology)
> >
> >  bool MediaDevice::populateLinks(const struct media_v2_topology &topology)
> >  {
> > -	media_v2_link *mediaLinks = reinterpret_cast<media_v2_link *>
> > -				    (topology.ptr_links);
> > +	struct media_v2_link *mediaLinks = reinterpret_cast<struct media_v2_link *>
> > +							   (topology.ptr_links);
> >
> >  	for (unsigned int i = 0; i < topology.num_links; ++i) {
> >  		/*
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190103/2c5852cd/attachment-0001.sig>