[libcamera-devel,1/4] libcamera: ipa_proxy: use utils::split()

Message ID 20200317173145.18226-2-kgupta@es.iitr.ac.in
State Superseded
Headers show
Series
  • libcamera: determine IPA_PROXY_PATH at runtime
Related show

Commit Message

Kaaira Gupta March 17, 2020, 5:31 p.m. UTC
Replace the manual string splitting with utils::split()

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 src/libcamera/ipa_proxy.cpp | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Comments

Kieran Bingham March 17, 2020, 8:10 p.m. UTC | #1
Hi Kaaira,

On 17/03/2020 17:31, Kaaira Gupta wrote:
> Replace the manual string splitting with utils::split()
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  src/libcamera/ipa_proxy.cpp | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> index 5a3d2f1..e04117c 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -83,23 +83,16 @@ std::string IPAProxy::resolvePath(const std::string &file) const
>  
>  	/* No exec target in install directory; check env variable. */
>  	const char *execPaths = utils::secure_getenv("LIBCAMERA_IPA_PROXY_PATH");
> -	while (execPaths) {
> -		const char *delim = strchrnul(execPaths, ':');
> -		size_t count = delim - execPaths;
> -
> -		if (count) {
> -			std::string proxyPath(execPaths, count);
> +	if (execPaths) {
> +		for (const auto &dir : utils::split(execPaths, ":")){

Quite minor, but there should be a space between the )) and the { above

I.e. like this:

+		for (const auto &dir : utils::split(execPaths, ":")) {

We have a utility called checkstyle.py in the utils directory at the top
of the tree, which can be installed as a pre-commit hook to help catch
these when you create the patch.

>From your source directory, install the git hook with the following:

 cp utils/hooks/pre-commit .git/hooks/pre-commit

Though it will be useful to install clang-format first.


> +			if (dir.empty())
> +				continue;

I'd add a blank line separator here.

> +			std::string proxyPath = dir;
>  			proxyPath += proxyFile;
>  			if (!access(proxyPath.c_str(), X_OK))
>  				return proxyPath;
>  		}
> -
> -		if (*delim == '\0')
> -			break;
> -
> -		execPaths += count + 1;
>  	}
> -

I would keep this separating blank line here.

>  	return std::string();
>  }


Other than the minor whitespace issues there,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Patch

diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
index 5a3d2f1..e04117c 100644
--- a/src/libcamera/ipa_proxy.cpp
+++ b/src/libcamera/ipa_proxy.cpp
@@ -83,23 +83,16 @@  std::string IPAProxy::resolvePath(const std::string &file) const
 
 	/* No exec target in install directory; check env variable. */
 	const char *execPaths = utils::secure_getenv("LIBCAMERA_IPA_PROXY_PATH");
-	while (execPaths) {
-		const char *delim = strchrnul(execPaths, ':');
-		size_t count = delim - execPaths;
-
-		if (count) {
-			std::string proxyPath(execPaths, count);
+	if (execPaths) {
+		for (const auto &dir : utils::split(execPaths, ":")){
+			if (dir.empty())
+				continue;
+			std::string proxyPath = dir;
 			proxyPath += proxyFile;
 			if (!access(proxyPath.c_str(), X_OK))
 				return proxyPath;
 		}
-
-		if (*delim == '\0')
-			break;
-
-		execPaths += count + 1;
 	}
-
 	return std::string();
 }