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

[Niklas Söderlund]

Hi Jacopo,

On 2019-01-03 08:57:26 +0100, Jacopo Mondi wrote:
> 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'

Please do, for just the struct changes feel free to add

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> 
> 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



-- 
Regards,
Niklas Söderlund