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

[Niklas Söderlund]

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.

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

>  		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