| Message ID | 20190102120256.7769-2-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > 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@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@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund
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; @@ -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); 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) { /*
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@jmondi.org> --- src/libcamera/media_device.cpp | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) -- 2.20.1