| Message ID | 20210303232126.3369069-1-raj.khem@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Khem, Thank you for the patch. On Wed, Mar 03, 2021 at 03:21:26PM -0800, Khem Raj wrote: > With c++17 loop range bases are defined where copy is obvious since > iterator returns a copy and not reference, gcc11 will emit a warning > about this > > uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string<cha > r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct] > | 432 | for (const std::string &name : { "idVendor", "idProduct" }) { > | | ^~~~ Looks like I should add gcc 11 to my build tests :-) > Therefore making it explicit is better > > Signed-off-by: Khem Raj <raj.khem@gmail.com> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 031f96e2..ef23ece7 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -429,7 +429,7 @@ std::string PipelineHandlerUVC::generateId(const UVCCameraData *data) > > /* Creata a device ID from the USB devices vendor and product ID. */ > std::string deviceId; > - for (const std::string &name : { "idVendor", "idProduct" }) { > + for (const auto name : { "idVendor", "idProduct" }) { What would you think about making this for (const char *name : { "idVendor", "idProduct" }) { ? We tend to only use auto when the explicit type is either impossible to type, or would be too long, as an explicit type makes it easier for the reader to know the type of the variable. I however have to say that using auto here since the beginning would have prevented this bug from happening in the first place. I'll let you decide what you think is best, and if you opt for const char *name, I can change this when applying, there's no need to resend the patch. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > std::ifstream file(path + "/../" + name); > > if (!file.is_open())
Hi Khem, On Thu, Mar 04, 2021 at 02:00:37AM +0200, Laurent Pinchart wrote: > On Wed, Mar 03, 2021 at 03:21:26PM -0800, Khem Raj wrote: > > With c++17 loop range bases are defined where copy is obvious since > > iterator returns a copy and not reference, gcc11 will emit a warning > > about this > > > > uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string<cha > > r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct] > > | 432 | for (const std::string &name : { "idVendor", "idProduct" }) { > > | | ^~~~ > > Looks like I should add gcc 11 to my build tests :-) > > > Therefore making it explicit is better > > > > Signed-off-by: Khem Raj <raj.khem@gmail.com> > > --- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index 031f96e2..ef23ece7 100644 > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > @@ -429,7 +429,7 @@ std::string PipelineHandlerUVC::generateId(const UVCCameraData *data) > > > > /* Creata a device ID from the USB devices vendor and product ID. */ > > std::string deviceId; > > - for (const std::string &name : { "idVendor", "idProduct" }) { > > + for (const auto name : { "idVendor", "idProduct" }) { > > What would you think about making this > > for (const char *name : { "idVendor", "idProduct" }) { > > ? We tend to only use auto when the explicit type is either impossible > to type, or would be too long, as an explicit type makes it easier for > the reader to know the type of the variable. I however have to say that > using auto here since the beginning would have prevented this bug from > happening in the first place. I'll let you decide what you think is > best, and if you opt for const char *name, I can change this when > applying, there's no need to resend the patch. Would this change be OK for you ? > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > std::ifstream file(path + "/../" + name); > > > > if (!file.is_open())
On Mon, Mar 8, 2021 at 4:07 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Khem, > > On Thu, Mar 04, 2021 at 02:00:37AM +0200, Laurent Pinchart wrote: > > On Wed, Mar 03, 2021 at 03:21:26PM -0800, Khem Raj wrote: > > > With c++17 loop range bases are defined where copy is obvious since > > > iterator returns a copy and not reference, gcc11 will emit a warning > > > about this > > > > > > uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string<cha > > > r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct] > > > | 432 | for (const std::string &name : { "idVendor", "idProduct" }) { > > > | | ^~~~ > > > > Looks like I should add gcc 11 to my build tests :-) > > > > > Therefore making it explicit is better > > > > > > Signed-off-by: Khem Raj <raj.khem@gmail.com> > > > --- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > index 031f96e2..ef23ece7 100644 > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > @@ -429,7 +429,7 @@ std::string PipelineHandlerUVC::generateId(const UVCCameraData *data) > > > > > > /* Creata a device ID from the USB devices vendor and product ID. */ > > > std::string deviceId; > > > - for (const std::string &name : { "idVendor", "idProduct" }) { > > > + for (const auto name : { "idVendor", "idProduct" }) { > > > > What would you think about making this > > > > for (const char *name : { "idVendor", "idProduct" }) { > > > > ? We tend to only use auto when the explicit type is either impossible > > to type, or would be too long, as an explicit type makes it easier for > > the reader to know the type of the variable. I however have to say that > > using auto here since the beginning would have prevented this bug from > > happening in the first place. I'll let you decide what you think is > > best, and if you opt for const char *name, I can change this when > > applying, there's no need to resend the patch. > > Would this change be OK for you ? > I have to test this out and haven't yet got to it. but I think this change will be ok as well. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > std::ifstream file(path + "/../" + name); > > > > > > if (!file.is_open()) > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 031f96e2..ef23ece7 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -429,7 +429,7 @@ std::string PipelineHandlerUVC::generateId(const UVCCameraData *data) /* Creata a device ID from the USB devices vendor and product ID. */ std::string deviceId; - for (const std::string &name : { "idVendor", "idProduct" }) { + for (const auto name : { "idVendor", "idProduct" }) { std::ifstream file(path + "/../" + name); if (!file.is_open())
With c++17 loop range bases are defined where copy is obvious since iterator returns a copy and not reference, gcc11 will emit a warning about this uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string<cha r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct] | 432 | for (const std::string &name : { "idVendor", "idProduct" }) { | | ^~~~ Therefore making it explicit is better Signed-off-by: Khem Raj <raj.khem@gmail.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)