You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "pussuw (via GitHub)" <gi...@apache.org> on 2023/04/05 10:02:00 UTC
[GitHub] [nuttx-apps] pussuw commented on a diff in pull request #1706: nsh/alias: Add support for alias arguments
pussuw commented on code in PR #1706:
URL: https://github.com/apache/nuttx-apps/pull/1706#discussion_r1158302705
##########
nshlib/nsh_parse.c:
##########
@@ -59,11 +59,29 @@
# define HAVE_MEMLIST 1
#endif
+/* If CONFIG_NSH_ALIAS is enabled, the alias strings might need dynamic
+ * memory, in case the alias has arguments and is set like:
+ *
+ * $ alias ls='ls -l'
+ *
+ * In this case the command verb and the arguments need to be separated, much
+ * like the argument separation is done with environment variable expansion.
+ *
+ * This needs a new working buffer in order to keep the original alias string
+ * intact.
+ */
+
+#ifdef CONFIG_NSH_ALIAS
+# define ALIAS_ALLOCS 2
+#else
+# define ALIAS_ALLOCS 0
+#endif
+
#if defined(HAVE_MEMLIST) && !defined(CONFIG_NSH_MAXALLOCS)
# ifdef CONFIG_NSH_ARGCAT
-# define CONFIG_NSH_MAXALLOCS (2*CONFIG_NSH_MAXARGUMENTS)
+# define CONFIG_NSH_MAXALLOCS ((2*CONFIG_NSH_MAXARGUMENTS) + ALIAS_ALLOCS)
Review Comment:
I like the suggestion but the reason I did not add the spaces originally was that it would exceed the 78 character limit.
```
# define CONFIG_NSH_MAXALLOCS ((2 * CONFIG_NSH_MAXARGUMENTS) + \
ALIAS_ALLOCS)
```
So it is a trade-off, add spaces to improve readability but add extra white space, or keep it like it is.
The original code did not have the whitespaces so that is why I did not add them + the line change myself.
What do you think, which is better ? I'm fine either way.
##########
nshlib/nsh_parse.c:
##########
@@ -59,11 +59,29 @@
# define HAVE_MEMLIST 1
#endif
+/* If CONFIG_NSH_ALIAS is enabled, the alias strings might need dynamic
+ * memory, in case the alias has arguments and is set like:
+ *
+ * $ alias ls='ls -l'
+ *
+ * In this case the command verb and the arguments need to be separated, much
+ * like the argument separation is done with environment variable expansion.
+ *
+ * This needs a new working buffer in order to keep the original alias string
+ * intact.
+ */
+
+#ifdef CONFIG_NSH_ALIAS
+# define ALIAS_ALLOCS 2
+#else
+# define ALIAS_ALLOCS 0
+#endif
+
#if defined(HAVE_MEMLIST) && !defined(CONFIG_NSH_MAXALLOCS)
# ifdef CONFIG_NSH_ARGCAT
-# define CONFIG_NSH_MAXALLOCS (2*CONFIG_NSH_MAXARGUMENTS)
+# define CONFIG_NSH_MAXALLOCS ((2*CONFIG_NSH_MAXARGUMENTS) + ALIAS_ALLOCS)
Review Comment:
I like the suggestion but the reason I did not add the spaces originally was that it would exceed the 78 character limit.
```
# define CONFIG_NSH_MAXALLOCS ((2 * CONFIG_NSH_MAXARGUMENTS) + \
ALIAS_ALLOCS)
```
So it is a trade-off, add spaces to improve readability but add extra white space, or keep it like it is.
The original code did not have the whitespaces so that is why I did not add them + the line change myself.
What do you think, which is better ? I'm fine either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org