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/06/01 15:21:41 UTC
[GitHub] [incubator-nuttx] patacongo opened a new pull request #1170: include/nuttx/video/video.h: Move global variable declaration out of header file
patacongo opened a new pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170
## Summary
Move global variable declaration out of include/nuttx/video/video.h and into the file where it is initialized. With some toolchains/environments, declaring globals in header files results in multiply defined symobl errors at link time. This corrects that build problem.
----------------------------------------------------------------
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 #1170: include/nuttx/video/video.h: Move global variable definition out of header file
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170#discussion_r433420248
##########
File path: boards/arm/cxd56xx/spresense/src/cxd56_bringup.c
##########
@@ -139,6 +139,14 @@
#endif
#endif
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#ifdef CONFIG_VIDEO_ISX012
+FAR const struct video_devops_s *g_video_devops;
Review comment:
Okay.. I did change it as you asked. I really hate working with crap designs like 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] patacongo commented on a change in pull request #1170: include/nuttx/video/video.h: Move global variable definition out of header file
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170#discussion_r433415240
##########
File path: boards/arm/cxd56xx/spresense/src/cxd56_bringup.c
##########
@@ -139,6 +139,14 @@
#endif
#endif
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#ifdef CONFIG_VIDEO_ISX012
+FAR const struct video_devops_s *g_video_devops;
Review comment:
For now boards/arm/cxd56xx/spresense/src/cxd56_bringup.c is the correct place because there and only there is can be conditioned on #ifdef CONFIG_VIDEO_ISX012 and only if it used.
So I am not going to change the code now. It does need more work to be a respectable driver interface. It sucks now in general and I cannot really make that better. Really terrible, unprofessional code.
----------------------------------------------------------------
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 #1170: include/nuttx/video/video.h: Move global variable definition out of header file
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170#discussion_r433415240
##########
File path: boards/arm/cxd56xx/spresense/src/cxd56_bringup.c
##########
@@ -139,6 +139,14 @@
#endif
#endif
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#ifdef CONFIG_VIDEO_ISX012
+FAR const struct video_devops_s *g_video_devops;
Review comment:
For now boards/arm/cxd56xx/spresense/src/cxd56_bringup.c is the correct place because there and only there is can be conditioned on #ifdef CONFIG_VIDEO_ISX012 and only if it used.
So I am not going to change the code now. It does need more work to be a respectable driver interface. It sucks now in general and I cannot really make that better.
----------------------------------------------------------------
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 #1170: include/nuttx/video/video.h: Move global variable definition out of header file
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170#discussion_r433418915
##########
File path: boards/arm/cxd56xx/spresense/src/cxd56_bringup.c
##########
@@ -139,6 +139,14 @@
#endif
#endif
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#ifdef CONFIG_VIDEO_ISX012
+FAR const struct video_devops_s *g_video_devops;
Review comment:
Okay.. I did it anyway
----------------------------------------------------------------
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 #1170: include/nuttx/video/video.h: Move global variable definition out of header file
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170#discussion_r433411098
##########
File path: boards/arm/cxd56xx/spresense/src/cxd56_bringup.c
##########
@@ -139,6 +139,14 @@
#endif
#endif
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#ifdef CONFIG_VIDEO_ISX012
+FAR const struct video_devops_s *g_video_devops;
Review comment:
I thought about that, but then it could force video.o into the link even if it were not used. I also though about a separate C file that contains nothing but the g_video_devops definition.
The real solution is to remove the global variable altogether. It is very bad style for a driver interface. For one thing, it limits the use of the driver to a single instance. That is something that I have been careful to avoid in all drivers. This breaks that rule. See Issue #439. Anything else is as band-aid to work around a very bad design.
----------------------------------------------------------------
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 #1170: include/nuttx/video/video.h: Move global variable definition out of header file
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170#discussion_r433332874
##########
File path: boards/arm/cxd56xx/spresense/src/cxd56_bringup.c
##########
@@ -139,6 +139,14 @@
#endif
#endif
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#ifdef CONFIG_VIDEO_ISX012
+FAR const struct video_devops_s *g_video_devops;
Review comment:
should we put it to a common location?
drivers/video/video.c
It's very strange that the common code reference a symbole which is defined in board specific file.
----------------------------------------------------------------
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 #1170: include/nuttx/video/video.h: Move global variable definition out of header file
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170#discussion_r433415240
##########
File path: boards/arm/cxd56xx/spresense/src/cxd56_bringup.c
##########
@@ -139,6 +139,14 @@
#endif
#endif
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#ifdef CONFIG_VIDEO_ISX012
+FAR const struct video_devops_s *g_video_devops;
Review comment:
For now boards/arm/cxd56xx/spresense/src/cxd56_bringup.c is the correct place because there and only there is can be conditioned on #ifdef CONFIG_VIDEO_ISX012 and only if it used.
So I am not going to change the code now. It does need more work to be a respectable driver interface. It sucks now in general and I cannot really make that better. Really terrible, unprofessional code.
----------------------------------------------------------------
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 #1170: include/nuttx/video/video.h: Move global variable definition out of header file
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170#discussion_r433418915
##########
File path: boards/arm/cxd56xx/spresense/src/cxd56_bringup.c
##########
@@ -139,6 +139,14 @@
#endif
#endif
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#ifdef CONFIG_VIDEO_ISX012
+FAR const struct video_devops_s *g_video_devops;
Review comment:
Okay.. I did it anyway
----------------------------------------------------------------
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 #1170: include/nuttx/video/video.h: Move global variable definition out of header file
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170#discussion_r433420248
##########
File path: boards/arm/cxd56xx/spresense/src/cxd56_bringup.c
##########
@@ -139,6 +139,14 @@
#endif
#endif
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+#ifdef CONFIG_VIDEO_ISX012
+FAR const struct video_devops_s *g_video_devops;
Review comment:
Okay.. I did change it as you asked. I really hate working with crap designs like this.
As a general rule, I put the global variable definition in the file that initializes and exports the global variable. Putting the global variable definition in a file that imports the global variable breaks all common sense. But this is an idiotic design and violates EVERY basic principle of good module design and well-defined interfaces: Global variables are strictly forbidden in the implementation in intermodule interfaces. So this is total shit.
----------------------------------------------------------------
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] Ouss4 merged pull request #1170: include/nuttx/video/video.h: Move global variable definition out of header file
Posted by GitBox <gi...@apache.org>.
Ouss4 merged pull request #1170:
URL: https://github.com/apache/incubator-nuttx/pull/1170
----------------------------------------------------------------
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