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 2022/03/29 11:10:55 UTC
[GitHub] [incubator-nuttx-apps] Gary-Hobson opened a new pull request #1108: quickjs: fix compile warning
Gary-Hobson opened a new pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108
## Summary
optind is a global variable in getopt.h (a macro defined in stdlib.h in nuttx).
Here it is used as a variable, not a variable of getopt
## 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #1108: quickjs: fix compile warning
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108#issuecomment-1081769909
@Gary-Hobson please fix this warning:
```
Error: /home/runner/work/incubator-nuttx-apps/incubator-nuttx-apps/apps/interpreters/quickjs/qjsmini.c:393:10: error: Multiple data definitions
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on pull request #1108: quickjs: fix compile warning
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108#issuecomment-1081780218
I'm just saying that this PR highlights places in `boards/arm/lpc31xx/ea3131/tools/lpchdr.c`, `boards/arm/lpc31xx/ea3152/tools/lpchdr.c`, `boards/arm/lpc31xx/olimex-lpc-h3131/tools/lpchdr.c` and `optind++;` is not usable in NuttX environment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on pull request #1108: quickjs: fix compile warning
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108#issuecomment-1081810502
> > > > I just briefly searched in the code and see that `optind++;` is used in many places. Taking into account that `#define optind (*(getoptindp()))` this becomes a useless and all the code that is relying on `optind` after `optind++` is executed wrongly.
> > >
> > >
> > > `optind++` is good usage to access the argument after option. The problem here is that quickjs parse the argument by self and doesn't use getopt at all, but the local variable definition conflict with optind unintentionally.
> >
> >
> > @xiaoxiang781216 I'm talking not about the problem here. The author solves it by renaming of a variable. The case is that in Linux (https://linux.die.net/man/3/optind) the `optind` is `int optind` and referenced via `extern int optind`, but in NuttX it is `FAR int *getoptindp(void); /* Index into argv */` and `#define optind (*(getoptindp()))` so `optind++;` does not increment the index holder. Please try to run a small example locally
> > ```
> > #include <stdio.h>
> >
> > #define optind (*(getoptindp()))
> >
> > int *getoptindp(void)
> > {
> > static int a;
> > return &a;
> > }
> >
> > int main()
> > {
> > printf("Test: %d, %d", optind++, optind);
> >
> > return 0;
> > }
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > and you will see that zero is printed two times.
>
> But your test isn't good since compiler may evaluate the last argument first(compiler can evaluate the expression in any order). I change your printf to this:
>
> ```
> printf("Test: %d, %d\n", optind++, optind++);
> ```
>
> The output show that the last argument evaluate first:
>
> ```
> Test: 1, 0
> ```
>
> But of course, it may change in your environment since the spec say this is an undefined behavior and anything could happen.
Sorry I completely messed the things with pointer. Probably need to have a good rest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] pkarashchenko edited a comment on pull request #1108: quickjs: fix compile warning
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108#issuecomment-1081777521
> > I just briefly searched in the code and see that `optind++;` is used in many places. Taking into account that `#define optind (*(getoptindp()))` this becomes a useless and all the code that is relying on `optind` after `optind++` is executed wrongly.
>
> `optind++` is good usage to access the argument after option. The problem here is that quickjs parse the argument by self and doesn't use getopt at all, but the local variable definition conflict with optind unintentionally.
@xiaoxiang781216 I'm talking not about the problem here. The author solves it by renaming of a variable.
The case is that in Linux (https://linux.die.net/man/3/optind) the `optind` is `int optind` and referenced via `extern int optind`, but in NuttX it is `FAR int *getoptindp(void); /* Index into argv */` and `#define optind (*(getoptindp()))` so `optind++;` does not increment the index holder.
Please try to run a small example locally
```
#include <stdio.h>
#define optind (*(getoptindp()))
int *getoptindp(void)
{
static int a;
return &a;
}
int main()
{
printf("Test: %d, %d", optind++, optind);
return 0;
}
```
and you will see that zeros are printed two times.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on pull request #1108: quickjs: fix compile warning
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108#issuecomment-1081753098
I just briefly searched in the code and see that `optind++;` is used in many places. Taking into account that `#define optind (*(getoptindp()))` this becomes a useless and all the code that is relying on `optind` after `optind++` is executed wrongly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #1108: quickjs: fix compile warning
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108#issuecomment-1081812235
> Sorry I completely messed the things with pointer. Probably need to have a good rest.
I think so, look like you work harder than me:).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] pkarashchenko commented on pull request #1108: quickjs: fix compile warning
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108#issuecomment-1081777521
> > I just briefly searched in the code and see that `optind++;` is used in many places. Taking into account that `#define optind (*(getoptindp()))` this becomes a useless and all the code that is relying on `optind` after `optind++` is executed wrongly.
>
> `optind++` is good usage to access the argument after option. The problem here is that quickjs parse the argument by self and doesn't use getopt at all, but the local variable definition conflict with optind unintentionally.
@xiaoxiang781216 I'm talking not about the problem here. The author solves it by renaming of a variable.
The case is that in Linux (https://linux.die.net/man/3/optind) the `optind` is `int optind` and referenced via `extern int optind`, but in NuttX it is `FAR int *getoptindp(void); /* Index into argv */` and `#define optind (*(getoptindp()))` so `optind++;` does not increment the index holder.
Please try to run a small example locally
```
#include <stdio.h>
#define optind (*(getoptindp()))
int *getoptindp(void)
{
static int a;
return &a;
}
int main()
{
printf("Test: %d, %d", optind++, optind);
return 0;
}
```
and you will see that zero is printed two times.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #1108: quickjs: fix compile warning
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108#issuecomment-1081766756
> I just briefly searched in the code and see that `optind++;` is used in many places. Taking into account that `#define optind (*(getoptindp()))` this becomes a useless and all the code that is relying on `optind` after `optind++` is executed wrongly.
`optind++` is good usage to access the argument after option. The problem here is that quickjs parse the argument by self and doesn't use getopt at all, but the local variable definition conflict with optind unintentionally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #1108: quickjs: fix compile warning
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108#issuecomment-1081792696
> > > I just briefly searched in the code and see that `optind++;` is used in many places. Taking into account that `#define optind (*(getoptindp()))` this becomes a useless and all the code that is relying on `optind` after `optind++` is executed wrongly.
> >
> >
> > `optind++` is good usage to access the argument after option. The problem here is that quickjs parse the argument by self and doesn't use getopt at all, but the local variable definition conflict with optind unintentionally.
>
> @xiaoxiang781216 I'm talking not about the problem here. The author solves it by renaming of a variable. The case is that in Linux (https://linux.die.net/man/3/optind) the `optind` is `int optind` and referenced via `extern int optind`, but in NuttX it is `FAR int *getoptindp(void); /* Index into argv */` and `#define optind (*(getoptindp()))` so `optind++;` does not increment the index holder. Please try to run a small example locally
>
> ```
> #include <stdio.h>
>
> #define optind (*(getoptindp()))
>
> int *getoptindp(void)
> {
> static int a;
> return &a;
> }
>
> int main()
> {
> printf("Test: %d, %d", optind++, optind);
>
> return 0;
> }
> ```
>
> and you will see that zero is printed two times.
But your test isn't good since compiler may evaluate the last argument first(compiler can evaluate the expression in any order). I change your printf to this:
```
printf("Test: %d, %d\n", optind++, optind++);
```
The output show that the last argument evaluate first:
```
Test: 1, 0
```
But of course, it may change in your environment since the spec say this is an undefined behavior and anything could happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 merged pull request #1108: quickjs: fix compile warning
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #1108:
URL: https://github.com/apache/incubator-nuttx-apps/pull/1108
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org