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/10/22 12:59:15 UTC
[GitHub] [incubator-nuttx] yamt opened a new pull request #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
yamt opened a new pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064
For NuttX, it should always be 16.
## Summary
## Impact
## 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 a change in pull request #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r512509080
##########
File path: tools/Config.mk
##########
@@ -531,3 +531,22 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# If CC_TYPE is not set, try to guess.
+ifeq ($(CC_TYPE),)
Review comment:
I plan to move all toolchain specific option to arch/**/Toolchain.defs, it's the best place what I can imagine. Once the movement is done, all boards will automatically support many good feature(e.g. uClibc++ or libc++) without the boring configuration. Actaully, I have done the similar thing for header file search path and libm/libsup++ recently.
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r523773825
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -72,11 +72,16 @@ NM = $(CROSSDEV)nm
OBJCOPY = $(CROSSDEV)objcopy
OBJDUMP = $(CROSSDEV)objdump
+# Note: -fshort-wchar for the case where NuttX and the host OS have
+# differnt wchar_t. On Nuttx, it's uint16_t. On macOS, it's 32-bit.
CFLAGS := $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+ $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
Review comment:
how to handle other arch? it may more simpler to typedef wchar_t in C same as in C++ without -fshort-wchar.
----------------------------------------------------------------
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 #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r513677386
##########
File path: include/sys/types.h
##########
@@ -188,6 +188,9 @@ typedef intptr_t ptrdiff_t;
typedef uint16_t wchar_t;
#endif
+#if defined(__WCHAR_WIDTH__) && __WCHAR_WIDTH__ != 16
+#error __WCHAR_WIDTH__ should be 16
Review comment:
who define __WCHAR_WIDTH__?
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r514751017
##########
File path: tools/Config.mk
##########
@@ -531,3 +531,22 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# If CC_TYPE is not set, try to guess.
+ifeq ($(CC_TYPE),)
Review comment:
No, I will do it in this weekend.
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-820023028
Yes, I think so.
--
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 #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r513891193
##########
File path: include/sys/types.h
##########
@@ -188,6 +188,9 @@ typedef intptr_t ptrdiff_t;
typedef uint16_t wchar_t;
#endif
+#if defined(__WCHAR_WIDTH__) && __WCHAR_WIDTH__ != 16
+#error __WCHAR_WIDTH__ should be 16
Review comment:
But wchar_t is compiler defined value from wiki:
https://en.wikipedia.org/wiki/Wide_character
----------------------------------------------------------------
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] yamt commented on pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-764163736
----------------------------------------------------------------
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] yamt commented on a change in pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r563413873
##########
File path: include/sys/types.h
##########
@@ -188,6 +188,10 @@ typedef intptr_t ptrdiff_t;
typedef uint16_t wchar_t;
#endif
+#if defined(__WCHAR_WIDTH__) && __WCHAR_WIDTH__ != 16
+#warning __WCHAR_WIDTH__ should be 16 on NuttX. Your toolchain might need -fshort-wchar.
Review comment:
yes. it's why this PR is a draft.
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r517824908
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -72,11 +72,16 @@ NM = $(CROSSDEV)nm
OBJCOPY = $(CROSSDEV)objcopy
OBJDUMP = $(CROSSDEV)objdump
+# Note: -fshort-wchar for the case where NuttX and the host OS have
+# differnt wchar_t. On Nuttx, it's uint16_t. On macOS, it's 32-bit.
CFLAGS := $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+ $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
Review comment:
Does really CPPFLAGS require -fshort-wchar? it seem that CPPFLAGS is designed to not use any language specific flag.
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r517820294
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -72,11 +72,16 @@ NM = $(CROSSDEV)nm
OBJCOPY = $(CROSSDEV)objcopy
OBJDUMP = $(CROSSDEV)objdump
+# Note: -fshort-wchar for the case where NuttX and the host OS have
+# differnt wchar_t. On Nuttx, it's uint16_t. On macOS, it's 32-bit.
CFLAGS := $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+ $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
Review comment:
should we put -fshort-wchar to ARCHCPUFLAGS/ARCHCPUFLAGSXX?
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-762392941
@yamt does this patch still need?
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r517847619
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -72,11 +72,16 @@ NM = $(CROSSDEV)nm
OBJCOPY = $(CROSSDEV)objcopy
OBJDUMP = $(CROSSDEV)objdump
+# Note: -fshort-wchar for the case where NuttX and the host OS have
+# differnt wchar_t. On Nuttx, it's uint16_t. On macOS, it's 32-bit.
CFLAGS := $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+ $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
Review comment:
PREPROCESS is most used to process nx template file to generate c source files, so template files aren't C files, but the generated files are.
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-820021780
Oh, no it seem that Linux(arm-08) still has the warning.
--
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] yamt commented on a change in pull request #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r513808991
##########
File path: include/sys/types.h
##########
@@ -188,6 +188,9 @@ typedef intptr_t ptrdiff_t;
typedef uint16_t wchar_t;
#endif
+#if defined(__WCHAR_WIDTH__) && __WCHAR_WIDTH__ != 16
+#error __WCHAR_WIDTH__ should be 16
Review comment:
it's a compiler predefined macro.
----------------------------------------------------------------
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] yamt commented on a change in pull request #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r514616886
##########
File path: include/sys/types.h
##########
@@ -188,6 +188,9 @@ typedef intptr_t ptrdiff_t;
typedef uint16_t wchar_t;
#endif
+#if defined(__WCHAR_WIDTH__) && __WCHAR_WIDTH__ != 16
+#error __WCHAR_WIDTH__ should be 16
Review comment:
do you mean to make this #warnings instead of #error?
i guess the compiler we are using for arm is configured for linux and thus `__WCHAR_WIDTH__` = 32.
anyway 32 is commonly used for modern OSes except windows.
----------------------------------------------------------------
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] yamt commented on pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-766475921
> i will rebase later
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 #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r514387086
##########
File path: include/sys/types.h
##########
@@ -188,6 +188,9 @@ typedef intptr_t ptrdiff_t;
typedef uint16_t wchar_t;
#endif
+#if defined(__WCHAR_WIDTH__) && __WCHAR_WIDTH__ != 16
+#error __WCHAR_WIDTH__ should be 16
Review comment:
So, should you split the check to another patch, maybe you can print the related option too. But why gcc complain __WCHAR_WIDTH__ not equal to 16 for arm?
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r517847619
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -72,11 +72,16 @@ NM = $(CROSSDEV)nm
OBJCOPY = $(CROSSDEV)objcopy
OBJDUMP = $(CROSSDEV)objdump
+# Note: -fshort-wchar for the case where NuttX and the host OS have
+# differnt wchar_t. On Nuttx, it's uint16_t. On macOS, it's 32-bit.
CFLAGS := $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+ $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
Review comment:
PREPROCESS is most used to process nx template file to generate c source file, so template files aren't C files, but the generated files are.
----------------------------------------------------------------
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] yamt commented on a change in pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r517828776
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -72,11 +72,16 @@ NM = $(CROSSDEV)nm
OBJCOPY = $(CROSSDEV)objcopy
OBJDUMP = $(CROSSDEV)objdump
+# Note: -fshort-wchar for the case where NuttX and the host OS have
+# differnt wchar_t. On Nuttx, it's uint16_t. On macOS, it's 32-bit.
CFLAGS := $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+ $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
Review comment:
i don't know how CPPFLAGS is designed.
the majority of the current uses of PREPROCESS in the tree are for C files, it seems.
----------------------------------------------------------------
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 #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r512509080
##########
File path: tools/Config.mk
##########
@@ -531,3 +531,22 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# If CC_TYPE is not set, try to guess.
+ifeq ($(CC_TYPE),)
Review comment:
I plan to move all toolchain specific option to arch/**/Toolchain.defs, I think it's the best place what I can imagine. Once the movement is done, all board will automatically support many good feature(e.g. uClibc++/libc++) without the boring configuration.
----------------------------------------------------------------
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 #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r512490191
##########
File path: tools/Config.mk
##########
@@ -531,3 +531,22 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# If CC_TYPE is not set, try to guess.
+ifeq ($(CC_TYPE),)
Review comment:
it isn't good to place toolchain specific flag here, the better place is Toolchain.defs or Make.defs
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r514751017
##########
File path: tools/Config.mk
##########
@@ -531,3 +531,22 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# If CC_TYPE is not set, try to guess.
+ifeq ($(CC_TYPE),)
Review comment:
No, I will do it in this weekend. The daily work is busy, I can't find the enough free time to modify more than 200 files.
----------------------------------------------------------------
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] yamt commented on a change in pull request #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r514748560
##########
File path: tools/Config.mk
##########
@@ -531,3 +531,22 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# If CC_TYPE is not set, try to guess.
+ifeq ($(CC_TYPE),)
Review comment:
sounds like a plan
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-820021187
The change pass all CI, Is it ready for merge, @yamt ?
--
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] yamt commented on pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-766475921
> i will rebase later
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] yamt commented on a change in pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r517823367
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -72,11 +72,16 @@ NM = $(CROSSDEV)nm
OBJCOPY = $(CROSSDEV)objcopy
OBJDUMP = $(CROSSDEV)objdump
+# Note: -fshort-wchar for the case where NuttX and the host OS have
+# differnt wchar_t. On Nuttx, it's uint16_t. On macOS, it's 32-bit.
CFLAGS := $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+ $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
Review comment:
for C and CXX, we can.
but how about CPPFLAGS?
----------------------------------------------------------------
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] yamt commented on a change in pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r563413873
##########
File path: include/sys/types.h
##########
@@ -188,6 +188,10 @@ typedef intptr_t ptrdiff_t;
typedef uint16_t wchar_t;
#endif
+#if defined(__WCHAR_WIDTH__) && __WCHAR_WIDTH__ != 16
+#warning __WCHAR_WIDTH__ should be 16 on NuttX. Your toolchain might need -fshort-wchar.
Review comment:
yes. it's why this PR is a draft.
----------------------------------------------------------------
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] yamt commented on pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-720319468
----------------------------------------------------------------
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] yamt commented on a change in pull request #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r512496772
##########
File path: tools/Config.mk
##########
@@ -531,3 +531,22 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# If CC_TYPE is not set, try to guess.
+ifeq ($(CC_TYPE),)
Review comment:
i agree.
i just got tired to add this to every Make.defs and is trying to cheat. :-)
----------------------------------------------------------------
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r563177825
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -72,11 +72,16 @@ NM = $(CROSSDEV)nm
OBJCOPY = $(CROSSDEV)objcopy
OBJDUMP = $(CROSSDEV)objdump
+# Note: -fshort-wchar for the case where NuttX and the host OS have
+# differnt wchar_t. On Nuttx, it's uint16_t. On macOS, it's 32-bit.
CFLAGS := $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+ $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
Review comment:
but this patch alredy contain the same change:
https://github.com/apache/incubator-nuttx/commit/3fc06ff2d145adc0fc3e46306e3a9886fa436ed8
##########
File path: include/sys/types.h
##########
@@ -188,6 +188,10 @@ typedef intptr_t ptrdiff_t;
typedef uint16_t wchar_t;
#endif
+#if defined(__WCHAR_WIDTH__) && __WCHAR_WIDTH__ != 16
+#warning __WCHAR_WIDTH__ should be 16 on NuttX. Your toolchain might need -fshort-wchar.
Review comment:
github show many warning happen with this change, all need be fixed before merge.
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -72,11 +72,16 @@ NM = $(CROSSDEV)nm
OBJCOPY = $(CROSSDEV)objcopy
OBJDUMP = $(CROSSDEV)objdump
+# Note: -fshort-wchar for the case where NuttX and the host OS have
+# differnt wchar_t. On Nuttx, it's uint16_t. On macOS, it's 32-bit.
CFLAGS := $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+ $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
CXXFLAGS := $(ARCHWARNINGSXX) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGSXX) $(ARCHXXINCLUDES) $(ARCHDEFINESXX) $(EXTRAFLAGS) -pipe
-CPPFLAGS := $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS)
+ $(ARCHCPUFLAGSXX) $(ARCHXXINCLUDES) $(ARCHDEFINESXX) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
+CPPFLAGS := $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) \
+ -fshort-wchar
Review comment:
but this patch alredy contain the same change:
https://github.com/apache/incubator-nuttx/commit/3fc06ff2d145adc0fc3e46306e3a9886fa436ed8
could you rebase your branch?
----------------------------------------------------------------
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] yamt commented on a change in pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r514757260
##########
File path: tools/Config.mk
##########
@@ -531,3 +531,22 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# If CC_TYPE is not set, try to guess.
+ifeq ($(CC_TYPE),)
Review comment:
i meant, it sounds like a good plan. thank you for taking care of this.
----------------------------------------------------------------
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 edited a comment on pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-720325730
> unfortunately #2195 is not a fix for the problem.
> we are still using a wrong type for wchar_t.
But, C++ has the right type now. For C, I assume that how to define wchar_t is determined by library implementer.
----------------------------------------------------------------
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] yamt commented on a change in pull request #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r512498516
##########
File path: tools/Config.mk
##########
@@ -531,3 +531,22 @@ else
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
endif
ARCHXXINCLUDES += ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
+
+# If CC_TYPE is not set, try to guess.
+ifeq ($(CC_TYPE),)
Review comment:
how do you think about having, say, gcc.mk somewhere, so that every gcc-using Make.defs or Toolchain.defs can include it?
----------------------------------------------------------------
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] yamt commented on pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-764163736
----------------------------------------------------------------
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] yamt commented on a change in pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r523826282
##########
File path: boards/sim/sim/sim/scripts/Make.defs
##########
@@ -72,11 +72,16 @@ NM = $(CROSSDEV)nm
OBJCOPY = $(CROSSDEV)objcopy
OBJDUMP = $(CROSSDEV)objdump
+# Note: -fshort-wchar for the case where NuttX and the host OS have
+# differnt wchar_t. On Nuttx, it's uint16_t. On macOS, it's 32-bit.
CFLAGS := $(ARCHWARNINGS) $(ARCHOPTIMIZATION) \
- $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe
+ $(ARCHCPUFLAGS) $(ARCHINCLUDES) $(ARCHDEFINES) $(EXTRAFLAGS) -pipe \
+ -fshort-wchar
Review comment:
`-fshort-wchar` should just work for the majority of archs.
for the rest of archs which is not using clang or gcc, i dunno. i guess they don't have `__WCHAR_TYPE__` either.
i don't know if the issue (wchar_t type mismatch between the compiler and nuttx) exists there 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 pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-720308917
----------------------------------------------------------------
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] yamt commented on a change in pull request #2064: include/sys/types.h: Bail out on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#discussion_r513932827
##########
File path: include/sys/types.h
##########
@@ -188,6 +188,9 @@ typedef intptr_t ptrdiff_t;
typedef uint16_t wchar_t;
#endif
+#if defined(__WCHAR_WIDTH__) && __WCHAR_WIDTH__ != 16
+#error __WCHAR_WIDTH__ should be 16
Review comment:
yes.
the compiler knows wchar_t and thus can predefine `__WCHAR_WIDTH__`.
the problem here is that we are using compilers which don't know nuttx's wchar_t.
we can workaround it with -fshort-wchar.
----------------------------------------------------------------
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] yamt commented on pull request #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-820022832
@xiaoxiang781216 yes. it passes the ci just because it uses -Wno-cpp.
i guess i should make this to #error and try to fix fallouts.
--
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 #2064: include/sys/types.h: Warn on unexpected __WCHAR_WIDTH__
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2064:
URL: https://github.com/apache/incubator-nuttx/pull/2064#issuecomment-727587789
> > wchar library in NuttX should work with either 16bits or 32bits wchar_t.
>
> really?
>
Yes, I don't see there are any assumption that wchar_t must be 16bits.
> anyway, C and C++ should use the consistent wchar_t.
Yes, we need to ensure the invaraint.
----------------------------------------------------------------
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