You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/07/30 08:58:29 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
xiaoxiang781216 opened a new pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487
## Summary
1.Utilize the lookup table for classification
2.Change the remaining macro to normal functions
Here is the related email thread:
https://lists.apache.org/thread.html/ra2f3416909dd64c22e9a194f293a7d1450d04904f7e3f64a400829b0%40%3Cdev.nuttx.apache.org%3E
## Impact
More confirm to the standard and other libc implementation
## Testing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#issuecomment-668345144
Close, https://github.com/apache/incubator-nuttx/pull/1496 is better for the embeded system.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463019472
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#undef EXTERN
+#ifdef __cplusplus
+#define EXTERN extern "C"
+extern "C"
+{
+#else
+#define EXTERN extern
+#endif
+
+extern const IPTR char _ctype_[];
Review comment:
```suggestion
extern const IOBJ char _ctype_[];
```
I think that IOBJ, not IPTR is the correct qualifier here. IOBJ indicates the storage location (__flash for AVR) and IPTR indicates memory acess method. See Example at: https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463021157
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#undef EXTERN
+#ifdef __cplusplus
+#define EXTERN extern "C"
+extern "C"
+{
+#else
+#define EXTERN extern
+#endif
+
+extern const IPTR char _ctype_[];
Review comment:
I wonder if either is needed if the ROM memory is explicitly accessed via up_romgetc().
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463639071
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
Review comment:
>
>
> If so, we can directly remove up_romgetc.
Yes, I think we can remove it. I think we might break some AVR configurations that might not have the linker script setup to correctly place the objects. I will need to look.
....
IOBJ and IPTR are defined only if CONFIG_AVR_HAS_MEMX_PTR is defined. So if CONFIG_AVR_HAS_MEMX_PTR is defined, the __flash storage class generates a section named .progmem. .rodata should be RAM and .progmem should be in FLASH. Otherwise, all data should in RAM. Based on this I think we are OK. I don't think any linker scripts need to change now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463212213
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> > There should be neglible use of ctype functions within the OS. Very little string processing is done by the OS itself. So it seems very wasteful to force such a large step in code side for functionality that is not used much in the OS.
>
> If we ignore libs/ and tools/, there is almost no use of ctype in the OS. Perhaps we should have a kctype.h header file for these. That avoids the mix you dislike:
>
> ./boards/mips/pic32mx/sure-pic32mx/src/pic32mx_lcd1602.c:#include <ctype.h>
> ./boards/renesas/m16c/skp16c26/src/m16c_lcd.c:#include <ctype.h>
> ./drivers/audio/tone.c:#include <ctype.h>
> ./drivers/syslog/syslog_rpmsg.c:#include <ctype.h>
> ./fs/dirent/fs_opendir.c:#include <ctype.h>
> ./fs/fat/fs_fat32dirent.c:#include <ctype.h>
>
> There might be other sneak paths through libc that include ctype.h, but generally I would say that is a pretty high price to pay to such little value to the OS.
>
> The libc usage of ctype is not in functions normally used by the OS either.
If so, how about change all macro to normal function? There are at least two perfect solution to resolve this issue:
1.Change all macro to inline function
2.Use the compiler extension:
https://stackoverflow.com/questions/1238016/are-compound-statements-blocks-surrounded-by-parens-expressions-in-ansi-c
But we can't use them due to the old compiler. In any case, the correction is always the first priority. If the result is wrong, the size or speed don't have any meaning.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463164814
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> For the PROTECTED build, this will draw in two copies of the lookup table. That doubles the FLASH overhead.
>
Yes, but all libc funcitons called by both userspace and kernel duplicate twice, the lookup table don't generate the huge difference here.
> I think that we should condition this on defined(CONFIG_BUILD_FLAT) || !defined(**KERNEL**). In the PROTECTED kernel build there should be very few ctyple reference and I think that they should be handled in a way that does not cause such a large overhead.
But this make the same code behaviour different in user space and kernel space. And does we(or user) really care about 256 ROM bytes addition? because the size explosion is much bigger than 256 bytes after we switch from flat mode to kernel/procted mode.
##########
File path: libs/libc/ctype/lib_ctype.c
##########
@@ -0,0 +1,75 @@
+/****************************************************************************
+ * libs/libc/assert/lib_ctype.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <ctype.h>
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+const IPTR char _ctype_[1 + 256 + 1] =
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463632575
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
Review comment:
If so, we can directly remove up_romgetc.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463023684
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
For the PROTECTED build, this will draw in two copies of the lookup table. That doubles the FLASH overhead.
I think that we should condition this on defined(CONFIG_BUILD_FLAT) || !defined(__KERNEL__). In the PROTECTED kernel build there should be very few ctyple reference and I think that they should be handled in a way that does not cause such a large overhead.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463157439
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
Review comment:
Yes, up_romget need move from arch/ to libc/machine, we need another patch for that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463022351
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
Review comment:
The AVR up_romgetc() is not accessible in user-space for PROTECTED and KERNEL builds. It should should be!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463157439
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
Review comment:
Yes, up_romget need move from arch/ to libc/machine.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463204517
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> There should be neglible use of ctype functions within the OS. Very little string processing is done by the OS itself. So it seems very wasteful to force such a large step in code side for functionality that is not used much in the OS.
If we ignore libs/ and tools/, there is almost no use of ctype in the OS. Perhaps we should have a kctype.h header file for these:
./boards/mips/pic32mx/sure-pic32mx/src/pic32mx_lcd1602.c:#include <ctype.h>
./boards/renesas/m16c/skp16c26/src/m16c_lcd.c:#include <ctype.h>
./drivers/audio/tone.c:#include <ctype.h>
./drivers/syslog/syslog_rpmsg.c:#include <ctype.h>
./fs/dirent/fs_opendir.c:#include <ctype.h>
./fs/fat/fs_fat32dirent.c:#include <ctype.h>
There might be other sneak paths through libc that include ctype.h, but generally I would say that is a pretty high price to pay to such little value to the OS.
The libc usage of ctype is not in functions normally used by the OS either.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463204517
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> There should be neglible use of ctype functions within the OS. Very little string processing is done by the OS itself. So it seems very wasteful to force such a large step in code side for functionality that is not used much in the OS.
If we ignore libs/ and tools/, there is almost no use of ctype in the OS. Perhaps we should have a kctype.h header file for these. That avoids the mix you dislike:
./boards/mips/pic32mx/sure-pic32mx/src/pic32mx_lcd1602.c:#include <ctype.h>
./boards/renesas/m16c/skp16c26/src/m16c_lcd.c:#include <ctype.h>
./drivers/audio/tone.c:#include <ctype.h>
./drivers/syslog/syslog_rpmsg.c:#include <ctype.h>
./fs/dirent/fs_opendir.c:#include <ctype.h>
./fs/fat/fs_fat32dirent.c:#include <ctype.h>
There might be other sneak paths through libc that include ctype.h, but generally I would say that is a pretty high price to pay to such little value to the OS.
The libc usage of ctype is not in functions normally used by the OS either.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463157048
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#undef EXTERN
+#ifdef __cplusplus
+#define EXTERN extern "C"
+extern "C"
+{
+#else
+#define EXTERN extern
+#endif
+
+extern const IPTR char _ctype_[];
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 closed pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463206201
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> But the standard compliant is the most INVIOLABLES rule.
We do not have to follow any external standards within the OS. POSIX and OpenGroup.org ONLY define the interface between applications and the OS. Within the OS we may do whatever we need to do to best support deeply embedded solutions. So the INVIOLABLES.txt statement is irrelevant in the particular discussion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463181137
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> But this make the same code behaviour different in user space and kernel space. And does we(or user) really care about 256 ROM bytes addition? because the size explosion is much bigger than 256 bytes after we switch from flat mode to kernel/protected mode.
I am not sure if that is a good argument or not. Systems become bloated and too large over time due to relatively small code size increased like this. The only way to prevent code bloat is to be vigilant to keep the code growth down.
There should be neglible use of ctype functions within the OS. Very little string processing is done by the OS itself. So it seems very wasteful to force such a large step in code side for functionality that is not used much in the OS.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463208769
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
Linux works the same way. It does not use the standard libc header files. Rather, it is has its own set of header files that are in parallel with the libc header files with alternative definitions for use within the kernel. It is a very standard thing to do within operating systems.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463197158
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
But the standard compliant is the most INVIOLABLES rule. To achieve this, we even don't allow the user to turn off signal and file system as I remember correctly. The correction is always the first priority, if the size is more important than speed, I would suggest to change all macro to normal function. otherwise we can keep the macro but add the table to avoid double evaluation. The mix just make the thing worse.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463639071
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
Review comment:
>
>
> If so, we can directly remove up_romgetc.
Yes, I think we can remove it. I think we might break some AVR configurations that might not have the linker script setup to correctly place the objects. I will need to look.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Brennan Ashton <ba...@brennanashton.com>.
On Thu, Jul 30, 2020, 1:35 PM Adam Feuer <ad...@starcat.io> wrote:
> I didn't know this, but storing artifacts for 90 days is free for open
> source projects on Github.
>
> This action can be used to upload the artifact:
> https://github.com/actions/upload-artifact
>
> Looks like there is a REST API for listing and downloading artifacts:
> https://developer.github.com/v3/actions/artifacts/
>
> And the Github CLI can probably use those APIs: https://cli.github.com/
Yes we already use that functionally for storing the code buddle used for
the test run (os,apps,testing)
--Brennan
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Adam Feuer <ad...@starcat.io>.
I didn't know this, but storing artifacts for 90 days is free for open
source projects on Github.
This action can be used to upload the artifact:
https://github.com/actions/upload-artifact
Looks like there is a REST API for listing and downloading artifacts:
https://developer.github.com/v3/actions/artifacts/
And the Github CLI can probably use those APIs: https://cli.github.com/
So as Brennan says, one way to do it would be to store the artifact and
some metadata about it after every run. Then have something that would
download metadata, compare, and report.
-adam
On Thu, Jul 30, 2020 at 1:17 PM Brennan Ashton <ba...@brennanashton.com>
wrote:
> On Thu, Jul 30, 2020, 1:13 PM Gregory Nutt <sp...@gmail.com> wrote:
>
> >
> > >
> > >>> It is a simple matter to see the cost of a PR if we add bloaty
> > >>> (https://github.com/google/bloaty) to ci.
> > >>>
> > >> Great please file the simple PR to add it. This is not trivial as we
> > >> need
> > >> to manage the artifacts from previous builds to do the comparison or
> > >> double
> > >> the build time. We heard you last time, but someone has to do to the
> > >> actual
> > >> work.
> > >>
> > > The absolute size of a build by itself is not so useful. You would
> > > need the size BEFORE the change (on the same base) for comparison
> > > with the size AFTER the change. I assume that bloaty does not have
> > > this A to B comparison built in?
> >
> > I am not a bloaty user so I might not be properly informed. But from
> > what I see, it is used to determine the cause of code bloat by analyzing
> > the .elf files. I think that would be a second step. A first, more
> > trivial step would be to simply detect the size increase, perhaps just
> > running 'size nuttx' and comparing to ... what? I imagine we would have
> > to keep some artifacts from a previous nightly built to find the A size
> > for the A-to-B comparison. If we detect a size increase, then bloaty
> > should tell use why.
>
>
> It will do both so might as well just save the output of it. Basically on
> the CI job on merge into master we would generate the size/bloaty report
> and save these artifacts next to the build (we do this for the source
> bundle already)
>
> Then when the PR runs it would fetch this data from the master build and
> pass it to bloaty to run the comparison report for the given build and
> report it in the comments or something.
>
--
Adam Feuer <ad...@starcat.io>
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Brennan Ashton <ba...@brennanashton.com>.
On Thu, Jul 30, 2020, 1:13 PM Gregory Nutt <sp...@gmail.com> wrote:
>
> >
> >>> It is a simple matter to see the cost of a PR if we add bloaty
> >>> (https://github.com/google/bloaty) to ci.
> >>>
> >> Great please file the simple PR to add it. This is not trivial as we
> >> need
> >> to manage the artifacts from previous builds to do the comparison or
> >> double
> >> the build time. We heard you last time, but someone has to do to the
> >> actual
> >> work.
> >>
> > The absolute size of a build by itself is not so useful. You would
> > need the size BEFORE the change (on the same base) for comparison
> > with the size AFTER the change. I assume that bloaty does not have
> > this A to B comparison built in?
>
> I am not a bloaty user so I might not be properly informed. But from
> what I see, it is used to determine the cause of code bloat by analyzing
> the .elf files. I think that would be a second step. A first, more
> trivial step would be to simply detect the size increase, perhaps just
> running 'size nuttx' and comparing to ... what? I imagine we would have
> to keep some artifacts from a previous nightly built to find the A size
> for the A-to-B comparison. If we detect a size increase, then bloaty
> should tell use why.
It will do both so might as well just save the output of it. Basically on
the CI job on merge into master we would generate the size/bloaty report
and save these artifacts next to the build (we do this for the source
bundle already)
Then when the PR runs it would fetch this data from the master build and
pass it to bloaty to run the comparison report for the given build and
report it in the comments or something.
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Gregory Nutt <sp...@gmail.com>.
>
>>> It is a simple matter to see the cost of a PR if we add bloaty
>>> (https://github.com/google/bloaty) to ci.
>>>
>> Great please file the simple PR to add it. This is not trivial as we
>> need
>> to manage the artifacts from previous builds to do the comparison or
>> double
>> the build time. We heard you last time, but someone has to do to the
>> actual
>> work.
>>
> The absolute size of a build by itself is not so useful. You would
> need the size BEFORE the change (on the same base) for comparison
> with the size AFTER the change. I assume that bloaty does not have
> this A to B comparison built in?
I am not a bloaty user so I might not be properly informed. But from
what I see, it is used to determine the cause of code bloat by analyzing
the .elf files. I think that would be a second step. A first, more
trivial step would be to simply detect the size increase, perhaps just
running 'size nuttx' and comparing to ... what? I imagine we would have
to keep some artifacts from a previous nightly built to find the A size
for the A-to-B comparison. If we detect a size increase, then bloaty
should tell use why.
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Brennan Ashton <ba...@brennanashton.com>.
On Thu, Jul 30, 2020, 1:02 PM Gregory Nutt <sp...@gmail.com> wrote:
>
> >> It is a simple matter to see the cost of a PR if we add bloaty
> >> (https://github.com/google/bloaty) to ci.
> >>
> > Great please file the simple PR to add it. This is not trivial as we need
> > to manage the artifacts from previous builds to do the comparison or
> double
> > the build time. We heard you last time, but someone has to do to the
> actual
> > work.
> >
> The absolute size of a build by itself is not so useful. You would need
> the size BEFORE the change (on the same base) for comparison with the
> size AFTER the change. I assume that bloaty does not have this A to B
> comparison built in?
>
Exactly which is why we would need to store the bloaty information on
master so the CI could fetch it and show the changes. This is why it's not
"simple".
Bloaty supports the smart comparison, but you have to supply the
information from it's last run. I'm happy to put it on my backlog I think
it's a good idea, but it won't happen from me for some time.
--Brennan
>
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Gregory Nutt <sp...@gmail.com>.
>> It is a simple matter to see the cost of a PR if we add bloaty
>> (https://github.com/google/bloaty) to ci.
>>
> Great please file the simple PR to add it. This is not trivial as we need
> to manage the artifacts from previous builds to do the comparison or double
> the build time. We heard you last time, but someone has to do to the actual
> work.
>
The absolute size of a build by itself is not so useful. You would need
the size BEFORE the change (on the same base) for comparison with the
size AFTER the change. I assume that bloaty does not have this A to B
comparison built in?
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Gregory Nutt <sp...@gmail.com>.
On 7/31/2020 8:44 AM, Xiang Xiao wrote:
> Macro version still evaluate the argument more than once regardless you name it kisdigit or isdigit. I am fine with any following combination:
> 1.lookup table for both userspace and kernel
> 2.normal function for both userspace and kernel
> 3.lookup table for userspace and normal function for kernel
> But Macro which can't guarantee to evaluate the argument only once isn't a good candidate even it is just used in kernel space.
#3 seems fine. Personally, I am not opposed to the allowing the
argument to be re-evaluated within the kernel. It is not an environment
for the casual user and there are are only a half dozen or so uses of
ctype functions. And we already have dozens of other cases:
This definitions appears in 107 files:
#define MIN(a,b) (((a) < (b)) ? (a) : (b))
And this appears in 85 files:
#define MAX(a,b) (((a) > (b)) ? (a) : (b))
I do want to make the user environment as safe and as usable as
possible, but programming within the OS is not for the weak at heart.
But #3 seems fine to me also.
RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by Xiang Xiao <xi...@gmail.com>.
Macro version still evaluate the argument more than once regardless you name it kisdigit or isdigit. I am fine with any following combination:
1.lookup table for both userspace and kernel
2.normal function for both userspace and kernel
3.lookup table for userspace and normal function for kernel
But Macro which can't guarantee to evaluate the argument only once isn't a good candidate even it is just used in kernel space.
> -----Original Message-----
> From: Gregory Nutt <sp...@gmail.com>
> Sent: Friday, July 31, 2020 9:57 PM
> To: dev@nuttx.apache.org
> Subject: Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to
> evaluate the argument more than once
>
>
> > .... My size concern is only in the PROTECTED build mode. In that
> > case, the ROM table would be duplicated: Once in user-space and once
> > in kernel space. That is the wasteful part because the OS does NOT do
> > any significant string processing and is a case where there would be
> > an unnecessary size penalty.
> >
> > If we ignore libs/ and tools/, there is almost no use of ctype.h in
> > the OS:
> >
> > ./boards/mips/pic32mx/sure-pic32mx/src/pic32mx_lcd1602.c:#include
> > <ctype.h>
> > ./boards/renesas/m16c/skp16c26/src/m16c_lcd.c:#include <ctype.h>
> > ./drivers/audio/tone.c:#include <ctype.h>
> > ./drivers/syslog/syslog_rpmsg.c:#include <ctype.h>
> > ./fs/dirent/fs_opendir.c:#include <ctype.h>
> > ./fs/fat/fs_fat32dirent.c:#include <ctype.h>
> >
> And, worse, most of these are not used in a "typical" configuration. But a couple are frequently included and that is enough to drag in
> the entire kernel-space copy of the lookup table into the build.
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Gregory Nutt <sp...@gmail.com>.
> .... My size concern is only in the PROTECTED build mode. In that
> case, the ROM table would be duplicated: Once in user-space and once
> in kernel space. That is the wasteful part because the OS does NOT do
> any significant string processing and is a case where there would be
> an unnecessary size penalty.
>
> If we ignore libs/ and tools/, there is almost no use of ctype.h in
> the OS:
>
> ./boards/mips/pic32mx/sure-pic32mx/src/pic32mx_lcd1602.c:#include
> <ctype.h>
> ./boards/renesas/m16c/skp16c26/src/m16c_lcd.c:#include <ctype.h>
> ./drivers/audio/tone.c:#include <ctype.h>
> ./drivers/syslog/syslog_rpmsg.c:#include <ctype.h>
> ./fs/dirent/fs_opendir.c:#include <ctype.h>
> ./fs/fat/fs_fat32dirent.c:#include <ctype.h>
>
And, worse, most of these are not used in a "typical" configuration.
But a couple are frequently included and that is enough to drag in the
entire kernel-space copy of the lookup table into the build.
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Gregory Nutt <sp...@gmail.com>.
> Yes, you are right: function version is still 20Bytes bigger than macro
> version. Do you have a better solution to fix this problem?
Just to remind people that my original size concern was not with the
table approach. I think the size if about a wash. Applications that do
a lot of string manipulations will benefit from the change more than
applications that do only a little -- which could even be penalized.
I personally would accept a single a single lookup table in user space.
My size concern is only in the PROTECTED build mode. In that case, the
ROM table would be duplicated: Once in user-space and once in kernel
space. That is the wasteful part because the OS does NOT do any
significant string processing and is a case where there would be an
unnecessary size penalty.
If we ignore libs/ and tools/, there is almost no use of ctype.h in the OS:
./boards/mips/pic32mx/sure-pic32mx/src/pic32mx_lcd1602.c:#include
<ctype.h>
./boards/renesas/m16c/skp16c26/src/m16c_lcd.c:#include <ctype.h>
./drivers/audio/tone.c:#include <ctype.h>
./drivers/syslog/syslog_rpmsg.c:#include <ctype.h>
./fs/dirent/fs_opendir.c:#include <ctype.h>
./fs/fat/fs_fat32dirent.c:#include <ctype.h>
There might be other sneak paths through libc that include ctype.h, but
generally I would say that is a pretty high price to pay to such little
value to the OS. The libc usage of ctype is not in functions normally
used by the OS.
So my proposal was you create some include/nuttx/kctype.h that has
definitions of similarly named functions implemented as simple macros
(like kisdigit() vs. isdigit()) or perhaps even global functions. The
internal OS programming environment does not need to have the creature
comforts of the user application environment. It is not intended to be
easy for application developments. This is Unix for god's sake!
Since the name spaces are separate in the PROTECTED build, we could even
use the standard naming include/nuttx/ctype.h, isdigit(), etc. But we
would need some extra conditioning on !defined (CONFIG_BUILD_FLAT) &&
defined (__KERNEL__).
RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by David Sidrane <Da...@nscdg.com>.
Xiang,
First let me say thank you for digging in to this.
The test case can relay influence the data.
The table is a step function. One call +256, the others are subject to
compiler magic.
If we think along the lines of scalability, the table is fine on BIG un
constrained systems. The calls are fine on MED systems and the macros are
fine on SMALL (tiny) constrained systems. I think that vector is aligned
with coding complexity and external lib usage as well. Would you agree, or
are you using 3rd party lib's on tiny procs? This is why I have maintained
the position it should be a Kconfig Knob.
David
-----Original Message-----
From: Xiang Xiao [mailto:xiaoxiang781216@gmail.com]
Sent: Friday, July 31, 2020 6:01 AM
To: dev@nuttx.apache.org
Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
change in pull request #1487: libc: Avoid ctype function to evaluate the
argument more than once
Yes, you are right: function version is still 20Bytes bigger than macro
version. Do you have a better solution to fix this problem?
> -----Original Message-----
> From: David Sidrane <Da...@nscdg.com>
> Sent: Friday, July 31, 2020 6:29 PM
> To: dev@nuttx.apache.org
> Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
> change in pull request #1487: libc: Avoid ctype function to
> evaluate the argument more than once
>
> > The normal function size is smaller than macro version.
>
> The comment and data seem to be in conflict.
>
> master:43255 is less than 'normal function':43275
>
> Am I missing something?
>
> -----Original Message-----
> From: Xiang Xiao [mailto:xiaoxiang781216@gmail.com]
> Sent: Friday, July 31, 2020 3:19 AM
> To: dev@nuttx.apache.org
> Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
> change in pull request #1487: libc: Avoid ctype function to
> evaluate the argument more than once
>
> Here is the size report for stm32f103-minimum:nsh:
> The master:
> text data bss dec hex filename
> 43255 108 2196 45559 b1f7 nuttx
> With lookup table
> patch(https://github.com/apache/incubator-nuttx/pull/1487):
> text data bss dec hex filename
> 43567 108 2196 45871 b32f nuttx
> With normal function
> patch(https://github.com/apache/incubator-nuttx/pull/1496):
> text data bss dec hex filename
> 43275 108 2196 45579 b20b nuttx
> The normal function size is smaller than macro version.
>
> > -----Original Message-----
> > From: Xiang Xiao <xi...@gmail.com>
> > Sent: Friday, July 31, 2020 4:08 PM
> > To: dev@nuttx.apache.org
> > Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
> > change in pull request #1487: libc: Avoid ctype function to evaluate
> > the argument more than once
> >
> > Let's restart the problem: it's definitely a bug that ctype macros
> > evaluate it's argument more than once. If the code size is more
> > important than the speed, let's change all macros to normal function
> > that we get the correction behavior and save the code size at the same
> > time.
> >
> > > -----Original Message-----
> > > From: David Sidrane <Da...@nscdg.com>
> > > Sent: Friday, July 31, 2020 7:32 AM
> > > To: dev@nuttx.apache.org
> > > Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on
> > > a change in pull request #1487: libc: Avoid ctype function to
> > > evaluate the argument more than once
> > >
> > > > Is this something we should be concerned about?
> > >
> > >
> > > Sorry if that came off wrong. I was agreeing with Greg's with the
> > > former statement. Not the latter one.
> > >
> > > David
> > >
> > > -----Original Message-----
> > > From: Brennan Ashton [mailto:bashton@brennanashton.com]
> > > Sent: Thursday, July 30, 2020 12:57 PM
> > > To: dev@nuttx.apache.org
> > > Subject: Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on
> > > a change in pull request #1487: libc: Avoid ctype function to
> > > evaluate the argument more than once
> > >
> > > On Thu, Jul 30, 2020, 12:40 PM David Sidrane
> > > <Da...@nscdg.com>
> > > wrote:
> > >
> > > > -1 on more bloat
> > > >
> > >
> > > That is not a constructive vote. No one thinks we should have more
> > > bloat.
> > >
> > > The question is do we need to focus more on this. Some of the
> > > changes are due to shortcuts we have taken in the past that are now
> > > causing issues. The PR that spurred this is exactly this. The code
> > > as it exists is wrong in that it has unexpected side effects which
> > > is what Xiao
> > is trying to address.
> > >
> > > I will acknowledge that there have been other changes where it was
> > > more of a complexity trade-off. Even on those there was discussion
> > > about the impact.
> > >
> > >
> > > > It is a simple matter to see the cost of a PR if we add bloaty
> > > > (https://github.com/google/bloaty) to ci.
> > > >
> > >
> > > Great please file the simple PR to add it. This is not trivial as we
> > > need to manage the artifacts from previous builds to do the
> > > comparison or double the build time. We heard you last time, but
> > someone has to do to the actual work.
> > >
> > > --Brennan
RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by Xiang Xiao <xi...@gmail.com>.
Yes, you are right: function version is still 20Bytes bigger than macro version. Do you have a better solution to fix this problem?
> -----Original Message-----
> From: David Sidrane <Da...@nscdg.com>
> Sent: Friday, July 31, 2020 6:29 PM
> To: dev@nuttx.apache.org
> Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to
> evaluate the argument more than once
>
> > The normal function size is smaller than macro version.
>
> The comment and data seem to be in conflict.
>
> master:43255 is less than 'normal function':43275
>
> Am I missing something?
>
> -----Original Message-----
> From: Xiang Xiao [mailto:xiaoxiang781216@gmail.com]
> Sent: Friday, July 31, 2020 3:19 AM
> To: dev@nuttx.apache.org
> Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to
> evaluate the argument more than once
>
> Here is the size report for stm32f103-minimum:nsh:
> The master:
> text data bss dec hex filename
> 43255 108 2196 45559 b1f7 nuttx
> With lookup table
> patch(https://github.com/apache/incubator-nuttx/pull/1487):
> text data bss dec hex filename
> 43567 108 2196 45871 b32f nuttx
> With normal function
> patch(https://github.com/apache/incubator-nuttx/pull/1496):
> text data bss dec hex filename
> 43275 108 2196 45579 b20b nuttx
> The normal function size is smaller than macro version.
>
> > -----Original Message-----
> > From: Xiang Xiao <xi...@gmail.com>
> > Sent: Friday, July 31, 2020 4:08 PM
> > To: dev@nuttx.apache.org
> > Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
> > change in pull request #1487: libc: Avoid ctype function to evaluate
> > the argument more than once
> >
> > Let's restart the problem: it's definitely a bug that ctype macros
> > evaluate it's argument more than once. If the code size is more
> > important than the speed, let's change all macros to normal function
> > that we get the correction behavior and save the code size at the same
> > time.
> >
> > > -----Original Message-----
> > > From: David Sidrane <Da...@nscdg.com>
> > > Sent: Friday, July 31, 2020 7:32 AM
> > > To: dev@nuttx.apache.org
> > > Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on
> > > a change in pull request #1487: libc: Avoid ctype function to
> > > evaluate the argument more than once
> > >
> > > > Is this something we should be concerned about?
> > >
> > >
> > > Sorry if that came off wrong. I was agreeing with Greg's with the
> > > former statement. Not the latter one.
> > >
> > > David
> > >
> > > -----Original Message-----
> > > From: Brennan Ashton [mailto:bashton@brennanashton.com]
> > > Sent: Thursday, July 30, 2020 12:57 PM
> > > To: dev@nuttx.apache.org
> > > Subject: Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on
> > > a change in pull request #1487: libc: Avoid ctype function to
> > > evaluate the argument more than once
> > >
> > > On Thu, Jul 30, 2020, 12:40 PM David Sidrane
> > > <Da...@nscdg.com>
> > > wrote:
> > >
> > > > -1 on more bloat
> > > >
> > >
> > > That is not a constructive vote. No one thinks we should have more
> > > bloat.
> > >
> > > The question is do we need to focus more on this. Some of the
> > > changes are due to shortcuts we have taken in the past that are now
> > > causing issues. The PR that spurred this is exactly this. The code
> > > as it exists is wrong in that it has unexpected side effects which
> > > is what Xiao
> > is trying to address.
> > >
> > > I will acknowledge that there have been other changes where it was
> > > more of a complexity trade-off. Even on those there was discussion
> > > about the impact.
> > >
> > >
> > > > It is a simple matter to see the cost of a PR if we add bloaty
> > > > (https://github.com/google/bloaty) to ci.
> > > >
> > >
> > > Great please file the simple PR to add it. This is not trivial as we
> > > need to manage the artifacts from previous builds to do the
> > > comparison or double the build time. We heard you last time, but
> > someone has to do to the actual work.
> > >
> > > --Brennan
RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by David Sidrane <Da...@nscdg.com>.
> The normal function size is smaller than macro version.
The comment and data seem to be in conflict.
master:43255 is less than 'normal function':43275
Am I missing something?
-----Original Message-----
From: Xiang Xiao [mailto:xiaoxiang781216@gmail.com]
Sent: Friday, July 31, 2020 3:19 AM
To: dev@nuttx.apache.org
Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
change in pull request #1487: libc: Avoid ctype function to evaluate the
argument more than once
Here is the size report for stm32f103-minimum:nsh:
The master:
text data bss dec hex filename
43255 108 2196 45559 b1f7 nuttx
With lookup table
patch(https://github.com/apache/incubator-nuttx/pull/1487):
text data bss dec hex filename
43567 108 2196 45871 b32f nuttx
With normal function
patch(https://github.com/apache/incubator-nuttx/pull/1496):
text data bss dec hex filename
43275 108 2196 45579 b20b nuttx
The normal function size is smaller than macro version.
> -----Original Message-----
> From: Xiang Xiao <xi...@gmail.com>
> Sent: Friday, July 31, 2020 4:08 PM
> To: dev@nuttx.apache.org
> Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
> change in pull request #1487: libc: Avoid ctype function to
> evaluate the argument more than once
>
> Let's restart the problem: it's definitely a bug that ctype macros
> evaluate it's argument more than once. If the code size is more
> important than the speed, let's change all macros to normal function that
> we get the correction behavior and save the code size at the
> same time.
>
> > -----Original Message-----
> > From: David Sidrane <Da...@nscdg.com>
> > Sent: Friday, July 31, 2020 7:32 AM
> > To: dev@nuttx.apache.org
> > Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
> > change in pull request #1487: libc: Avoid ctype function to evaluate
> > the argument more than once
> >
> > > Is this something we should be concerned about?
> >
> >
> > Sorry if that came off wrong. I was agreeing with Greg's with the former
> > statement. Not the latter one.
> >
> > David
> >
> > -----Original Message-----
> > From: Brennan Ashton [mailto:bashton@brennanashton.com]
> > Sent: Thursday, July 30, 2020 12:57 PM
> > To: dev@nuttx.apache.org
> > Subject: Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
> > change in pull request #1487: libc: Avoid ctype function to evaluate
> > the argument more than once
> >
> > On Thu, Jul 30, 2020, 12:40 PM David Sidrane <Da...@nscdg.com>
> > wrote:
> >
> > > -1 on more bloat
> > >
> >
> > That is not a constructive vote. No one thinks we should have more
> > bloat.
> >
> > The question is do we need to focus more on this. Some of the changes
> > are due to shortcuts we have taken in the past that are now causing
> > issues. The PR that spurred this is exactly this. The code as it exists
> > is wrong in that it has unexpected side effects which is what Xiao
> is trying to address.
> >
> > I will acknowledge that there have been other changes where it was
> > more of a complexity trade-off. Even on those there was discussion
> > about the impact.
> >
> >
> > > It is a simple matter to see the cost of a PR if we add bloaty
> > > (https://github.com/google/bloaty) to ci.
> > >
> >
> > Great please file the simple PR to add it. This is not trivial as we
> > need to manage the artifacts from previous builds to do the comparison
> > or double the build time. We heard you last time, but
> someone has to do to the actual work.
> >
> > --Brennan
RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by Xiang Xiao <xi...@gmail.com>.
Here is the size report for stm32f103-minimum:nsh:
The master:
text data bss dec hex filename
43255 108 2196 45559 b1f7 nuttx
With lookup table patch(https://github.com/apache/incubator-nuttx/pull/1487):
text data bss dec hex filename
43567 108 2196 45871 b32f nuttx
With normal function patch(https://github.com/apache/incubator-nuttx/pull/1496):
text data bss dec hex filename
43275 108 2196 45579 b20b nuttx
The normal function size is smaller than macro version.
> -----Original Message-----
> From: Xiang Xiao <xi...@gmail.com>
> Sent: Friday, July 31, 2020 4:08 PM
> To: dev@nuttx.apache.org
> Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to
> evaluate the argument more than once
>
> Let's restart the problem: it's definitely a bug that ctype macros evaluate it's argument more than once. If the code size is more
> important than the speed, let's change all macros to normal function that we get the correction behavior and save the code size at the
> same time.
>
> > -----Original Message-----
> > From: David Sidrane <Da...@nscdg.com>
> > Sent: Friday, July 31, 2020 7:32 AM
> > To: dev@nuttx.apache.org
> > Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
> > change in pull request #1487: libc: Avoid ctype function to evaluate
> > the argument more than once
> >
> > > Is this something we should be concerned about?
> >
> >
> > Sorry if that came off wrong. I was agreeing with Greg's with the former statement. Not the latter one.
> >
> > David
> >
> > -----Original Message-----
> > From: Brennan Ashton [mailto:bashton@brennanashton.com]
> > Sent: Thursday, July 30, 2020 12:57 PM
> > To: dev@nuttx.apache.org
> > Subject: Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
> > change in pull request #1487: libc: Avoid ctype function to evaluate
> > the argument more than once
> >
> > On Thu, Jul 30, 2020, 12:40 PM David Sidrane <Da...@nscdg.com>
> > wrote:
> >
> > > -1 on more bloat
> > >
> >
> > That is not a constructive vote. No one thinks we should have more bloat.
> >
> > The question is do we need to focus more on this. Some of the changes
> > are due to shortcuts we have taken in the past that are now causing
> > issues. The PR that spurred this is exactly this. The code as it exists is wrong in that it has unexpected side effects which is what Xiao
> is trying to address.
> >
> > I will acknowledge that there have been other changes where it was
> > more of a complexity trade-off. Even on those there was discussion about the impact.
> >
> >
> > > It is a simple matter to see the cost of a PR if we add bloaty
> > > (https://github.com/google/bloaty) to ci.
> > >
> >
> > Great please file the simple PR to add it. This is not trivial as we
> > need to manage the artifacts from previous builds to do the comparison or double the build time. We heard you last time, but
> someone has to do to the actual work.
> >
> > --Brennan
RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by Xiang Xiao <xi...@gmail.com>.
Let's restart the problem: it's definitely a bug that ctype macros evaluate it's argument more than once. If the code size is more important than the speed, let's change all macros to normal function that we get the correction behavior and save the code size at the same time.
> -----Original Message-----
> From: David Sidrane <Da...@nscdg.com>
> Sent: Friday, July 31, 2020 7:32 AM
> To: dev@nuttx.apache.org
> Subject: RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to
> evaluate the argument more than once
>
> > Is this something we should be concerned about?
>
>
> Sorry if that came off wrong. I was agreeing with Greg's with the former statement. Not the latter one.
>
> David
>
> -----Original Message-----
> From: Brennan Ashton [mailto:bashton@brennanashton.com]
> Sent: Thursday, July 30, 2020 12:57 PM
> To: dev@nuttx.apache.org
> Subject: Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to
> evaluate the argument more than once
>
> On Thu, Jul 30, 2020, 12:40 PM David Sidrane <Da...@nscdg.com>
> wrote:
>
> > -1 on more bloat
> >
>
> That is not a constructive vote. No one thinks we should have more bloat.
>
> The question is do we need to focus more on this. Some of the changes are due to shortcuts we have taken in the past that are now
> causing issues. The PR that spurred this is exactly this. The code as it exists is wrong in that it has unexpected side effects which is what
> Xiao is trying to address.
>
> I will acknowledge that there have been other changes where it was more of a complexity trade-off. Even on those there was
> discussion about the impact.
>
>
> > It is a simple matter to see the cost of a PR if we add bloaty
> > (https://github.com/google/bloaty) to ci.
> >
>
> Great please file the simple PR to add it. This is not trivial as we need to manage the artifacts from previous builds to do the
> comparison or double the build time. We heard you last time, but someone has to do to the actual work.
>
> --Brennan
RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by David Sidrane <Da...@nscdg.com>.
> Is this something we should be concerned about?
Sorry if that came off wrong. I was agreeing with Greg's with the former
statement. Not the latter one.
David
-----Original Message-----
From: Brennan Ashton [mailto:bashton@brennanashton.com]
Sent: Thursday, July 30, 2020 12:57 PM
To: dev@nuttx.apache.org
Subject: Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
change in pull request #1487: libc: Avoid ctype function to evaluate the
argument more than once
On Thu, Jul 30, 2020, 12:40 PM David Sidrane <Da...@nscdg.com>
wrote:
> -1 on more bloat
>
That is not a constructive vote. No one thinks we should have more bloat.
The question is do we need to focus more on this. Some of the changes are
due to shortcuts we have taken in the past that are now causing issues. The
PR that spurred this is exactly this. The code as it exists is wrong in
that it has unexpected side effects which is what Xiao is trying to
address.
I will acknowledge that there have been other changes where it was more of
a complexity trade-off. Even on those there was discussion about the
impact.
> It is a simple matter to see the cost of a PR if we add bloaty
> (https://github.com/google/bloaty) to ci.
>
Great please file the simple PR to add it. This is not trivial as we need
to manage the artifacts from previous builds to do the comparison or double
the build time. We heard you last time, but someone has to do to the actual
work.
--Brennan
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Brennan Ashton <ba...@brennanashton.com>.
On Thu, Jul 30, 2020, 12:40 PM David Sidrane <Da...@nscdg.com>
wrote:
> -1 on more bloat
>
That is not a constructive vote. No one thinks we should have more bloat.
The question is do we need to focus more on this. Some of the changes are
due to shortcuts we have taken in the past that are now causing issues. The
PR that spurred this is exactly this. The code as it exists is wrong in
that it has unexpected side effects which is what Xiao is trying to
address.
I will acknowledge that there have been other changes where it was more of
a complexity trade-off. Even on those there was discussion about the
impact.
> It is a simple matter to see the cost of a PR if we add bloaty
> (https://github.com/google/bloaty) to ci.
>
Great please file the simple PR to add it. This is not trivial as we need
to manage the artifacts from previous builds to do the comparison or double
the build time. We heard you last time, but someone has to do to the actual
work.
--Brennan
RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by David Sidrane <Da...@nscdg.com>.
-1 on more bloat
It is a simple matter to see the cost of a PR if we add bloaty
(https://github.com/google/bloaty) to ci.
-----Original Message-----
From: Gregory Nutt [mailto:spudaneco@gmail.com]
Sent: Thursday, July 30, 2020 12:25 PM
To: dev@nuttx.apache.org
Subject: Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a
change in pull request #1487: libc: Avoid ctype function to evaluate the
argument more than once
> But how long does it take you, when the linker tell you, you have over
> flowed .text by 256 bytes?
When you overflow memory due to a large number of modest size increases,
there is no way to recover. You are basically screwed because nothing
can really be done. The only way to prevent code bloat is by addressing
each and every modest size increase as they are introduced. That is the
only discipline that will keep the size of the OS within bounds.
BTW: This started out as a discussion in PR #1487. I am not sure how
it leaked into the dev list. But I think it is an important discussion
that all people associated with the project should discuss. We are
experiencing unbridled code growth now, especially since becoming an
Apache project. Each release is 1Kb or so larger that the previous
release on configurations that have no beneficial, functional
improvements. This is pure code bloat.
Certainly capabilities are increasing, but configurations that do not
exploit those new capabilities are increasing in size with absolutely no
benefit.
Is this something we should be concerned about? Or should be let it go
and just sweep every fractional Kb code increase under the rug where we
hide all of the things that we are too lazy to address? This is a
democracy. If you like it like that, that is cool.
Re: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by Gregory Nutt <sp...@gmail.com>.
> But how long does it take you, when the linker tell you, you have over
> flowed .text by 256 bytes?
When you overflow memory due to a large number of modest size increases,
there is no way to recover. You are basically screwed because nothing
can really be done. The only way to prevent code bloat is by addressing
each and every modest size increase as they are introduced. That is the
only discipline that will keep the size of the OS within bounds.
BTW: This started out as a discussion in PR #1487. I am not sure how
it leaked into the dev list. But I think it is an important discussion
that all people associated with the project should discuss. We are
experiencing unbridled code growth now, especially since becoming an
Apache project. Each release is 1Kb or so larger that the previous
release on configurations that have no beneficial, functional
improvements. This is pure code bloat.
Certainly capabilities are increasing, but configurations that do not
exploit those new capabilities are increasing in size with absolutely no
benefit.
Is this something we should be concerned about? Or should be let it go
and just sweep every fractional Kb code increase under the rug where we
hide all of the things that we are too lazy to address? This is a
democracy. If you like it like that, that is cool.
RE: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change
in pull request #1487: libc: Avoid ctype function to evaluate the argument
more than once
Posted by David Sidrane <Da...@nscdg.com>.
But how long does it take you, when the linker tell you, you have over
flowed .text by 256 bytes?
-----Original Message-----
From: GitBox [mailto:git@apache.org]
Sent: Thursday, July 30, 2020 12:13 PM
To: commits@nuttx.apache.org
Subject: [GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in
pull request #1487: libc: Avoid ctype function to evaluate the argument more
than once
xiaoxiang781216 commented on a change in pull request #1487:
URL:
https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463213449
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> Linux works the same way. It does not use the standard libc header
files. Rather, it is has its own set of header files that are in parallel
with the libc header files with alternative definitions for use within the
kernel. It is a very standard thing to do within operating systems.
We can use the special header file if you like, but the double evaluation
must be addressed. Nobody want to spend one/two day to debug this type of
error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463213449
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> Linux works the same way. It does not use the standard libc header files. Rather, it is has its own set of header files that are in parallel with the libc header files with alternative definitions for use within the kernel. It is a very standard thing to do within operating systems.
We can use the special header file if you like, but the double evaluation must be addressed. Nobody want to spend one/two day to debug this type of error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463213449
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> Linux works the same way. It does not use the standard libc header files. Rather, it is has its own set of header files that are in parallel with the libc header files with alternative definitions for use within the kernel. It is a very standard thing to do within operating systems.
We can use the special header file if you like, but the double evaluation must to address. no people want to spend one/two day to debug this type of error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463164814
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> For the PROTECTED build, this will draw in two copies of the lookup table. That doubles the FLASH overhead.
>
Yes, but all libc funcitons called by both userspace and kernel duplicate twice, the lookup table don't generate the huge difference here.
> I think that we should condition this on defined(CONFIG_BUILD_FLAT) || !defined(**KERNEL**). In the PROTECTED kernel build there should be very few ctyple reference and I think that they should be handled in a way that does not cause such a large overhead.
But this make the same code behaviour different in user space and kernel space. And does we(or user) really care about 256 ROM bytes addition? because the size explosion is much bigger than 256 bytes after we switch from flat mode to kernel/protected mode.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463021157
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#undef EXTERN
+#ifdef __cplusplus
+#define EXTERN extern "C"
+extern "C"
+{
+#else
+#define EXTERN extern
+#endif
+
+extern const IPTR char _ctype_[];
Review comment:
I wonder if either is needed if the ROM memory is explicitly accessed via up_romgetc(). In any event, IOBJ is need by the linker to force selected read-only data into ROM. All other read-only data must go into RAM.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463613932
##########
File path: include/ctype.h
##########
@@ -37,21 +37,50 @@
#ifndef __INCLUDE_CTYPE_H
#define __INCLUDE_CTYPE_H
-/* There is no consistent ctype implementation, just a smattering of
- * functions. Individually, they are okay, but a more standard, data lookup
- * approach would make more sense if used extensively.
- */
-
/****************************************************************************
* Included Files
****************************************************************************/
#include <nuttx/compiler.h>
+#include <nuttx/arch.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+/* The classification flags */
+
+#define _U 0x01 /* upper */
+#define _L 0x02 /* lower */
+#define _N 0x04 /* digit */
+#define _S 0x08 /* space */
+#define _P 0x10 /* punct */
+#define _C 0x20 /* cntrl */
+#define _X 0x40 /* a-f|A-F */
+#define _B 0x80 /* blank */
+
+#define __ctype_lookup(c) up_romgetc(__ctype_ + (c))
Review comment:
>
>
> Yes, up_romget need move from arch/ to libc/machine, we need another patch for that.
I don't think so. I don't think that the up_romgetc() is need at all if the IOBJ (__flash) storage class is used. Again, look at the code example at https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html
/* Locate array[] in flash memory */
const __flash int array[] = { 3, 5, 7, 11, 13, 17, 19 };
int i = 1;
int main (void)
{
/* Return 17 by reading from flash memory */
return array[array[i]];
}
If the IOBJ (aka __flash) storage class is used, then the compiler will automatically generate the correct assembly language sequence to access the data in I-Space. There is no need for up_romgetc() if that storage class is used.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463024646
##########
File path: libs/libc/ctype/lib_ctype.c
##########
@@ -0,0 +1,75 @@
+/****************************************************************************
+ * libs/libc/assert/lib_ctype.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <ctype.h>
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+const IPTR char _ctype_[1 + 256 + 1] =
Review comment:
```suggestion
const IOBJ char _ctype_[1 + 256 + 1] =
```
See above
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1487: libc: Avoid ctype function to evaluate the argument more than once
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1487:
URL: https://github.com/apache/incubator-nuttx/pull/1487#discussion_r463222216
##########
File path: include/ctype.h
##########
@@ -65,13 +94,10 @@
#ifdef __cplusplus
static inline int isspace(int c)
{
- return c == ' ' || c == '\t' || c == '\n' || c == '\r' ||
- c == '\f' || c == '\v';
+ return __ctype_lookup(c) & _S;
}
#else
-# define isspace(c) \
- ((c) == ' ' || (c) == '\t' || (c) == '\n' || (c) == '\r' || \
- (c) == '\f' || (c) == '\v')
+# define isspace(c) (__ctype_lookup(c) & _S)
Review comment:
> So the INVIOLABLES.txt statement is irrelevant in the particular discussion.
I meant that the first section of the INVIOLABLES.txt related to conforming to the portable POSIX application interface are not relative to this discussion. Certainly, the section on modularity and the internal architecture of the OS always relevant to a discussion of internal OS changes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org