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/07/13 03:39:46 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request, #6606: libc/stdio: enable long long formating by CONFIG_HAVE_LONG_LONG
xiaoxiang781216 opened a new pull request, #6606:
URL: https://github.com/apache/incubator-nuttx/pull/6606
## Summary
and remove CONFIG_LIBC_LONG_LONG option to simplify the usage.
note: the size will increase 668
before change:
```
text data bss dec hex filename
168440 348 4480 173268 2a4d4 nuttx
```
after change:
```
text data bss dec hex filename
169108 348 4480 173936 2a770 nuttx
```
## 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] xiaoxiang781216 commented on pull request #6606: libc/stdio: enable long long formating by CONFIG_HAVE_LONG_LONG
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6606:
URL: https://github.com/apache/incubator-nuttx/pull/6606#issuecomment-1184667749
Here is the new patch: https://github.com/apache/incubator-nuttx/pull/6613.
@ALTracer and @pkarashchenko please review it.
@ALTracer if you want to make HAVE_LONG_LONG configurable, please create a new PR for 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.
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] xiaoxiang781216 commented on pull request #6606: libc/stdio: enable long long formating by CONFIG_HAVE_LONG_LONG
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6606:
URL: https://github.com/apache/incubator-nuttx/pull/6606#issuecomment-1183968504
I will provide a patch to refine the implementation today, @ALTracer @pkarashchenko
--
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] xiaoxiang781216 commented on pull request #6606: libc/stdio: enable long long formating by CONFIG_HAVE_LONG_LONG
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6606:
URL: https://github.com/apache/incubator-nuttx/pull/6606#issuecomment-1183925230
> @xiaoxiang781216 I agree to fix the inconsistency that is currently present in the code, but IMO `CONFIG_HAVE_LONG_LONG` does not mean "use long long". I mean that it would be good to have an option to disable long long support if code needs to be tuned for small size even if compiler supports it. This is definitely not a scope of this PR, but do you consider this reasonable?
Yes, it's better to unify CONFIG_LIBC_LONG_LONG and CONFIG_HAVE_LONG_LONG into one option and selectable from Kconfig.
--
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] pkarashchenko commented on pull request #6606: libc/stdio: enable long long formating by CONFIG_HAVE_LONG_LONG
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6606:
URL: https://github.com/apache/incubator-nuttx/pull/6606#issuecomment-1183598893
@ALTracer Yes and that is exactly what I was posting in my comment above. Having decoupled `LIBC_LONG_LONG` and `HAVE_LONG_LONG` separately does not make sense because:
```
#ifdef CONFIG_HAVE_LONG_LONG
mount_sprintf(info, " %-10s %6llu%c %8llu%c %8llu%c %s\n", fstype,
size, sizelabel, used, usedlabel, free, freelabel,
mountpoint);
#else
mount_sprintf(info, " %-10s %6ld%c %8ld%c %8ld%c %s\n", fstype,
size, sizelabel, used, usedlabel, free, freelabel,
mountpoint);
#endif
```
```
/* Helper macro for printing lba addresses according to the sys/types.h */
#if defined(CONFIG_FS_LARGEFILE) && defined(CONFIG_HAVE_LONG_LONG)
#define PRIxLBA PRIx64
#define PRIdLBA PRId64
#else
#define PRIxLBA PRIx32
#define PRIdLBA PRId32
#endif
```
```
#if defined(CONFIG_FS_LARGEFILE) && defined(CONFIG_HAVE_LONG_LONG)
#define PRIdOFF PRId64
#define PRIiOFF PRIi64
#define PRIoOFF PRIo64
#define PRIuOFF PRIu64
#define PRIxOFF PRIx64
#define PRIXOFF PRIX64
#define SCNdOFF SCNd64
#define SCNiOFF SCNi64
#define SCNoOFF SCNo64
#define SCNuOFF SCNu64
#define SCNxOFF SCNx64
#else
#define PRIdOFF PRId32
#define PRIiOFF PRIi32
#define PRIoOFF PRIo32
#define PRIuOFF PRIu32
#define PRIxOFF PRIx32
#define PRIXOFF PRIX32
#define SCNdOFF SCNd32
#define SCNiOFF SCNi32
#define SCNoOFF SCNo32
#define SCNuOFF SCNu32
#define SCNxOFF SCNx32
#endif
```
so having those decoupled leads to inconsistency (I mean if `CONFIG_HAVE_LONG_LONG` is defined, but `LIBC_LONG_LONG` is not). This PR fixes the inconsistency and that has nothing to "key philosophy of NuttX being small/lightweight and configurable.", but just fixing the broken things.
The another part is something that you also pointed out is `CONFIG_HAVE_LONG_LONG` should be a `Kconfig` option and should be something that can be disabled to get better footprint and performance (small/lightweight). That means that we need to make a separate PR to fix 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.
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] pkarashchenko commented on pull request #6606: libc/stdio: enable long long formating by CONFIG_HAVE_LONG_LONG
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6606:
URL: https://github.com/apache/incubator-nuttx/pull/6606#issuecomment-1182828299
@xiaoxiang781216 I agree to fix the inconsistency that is currently present in the code, but IMO `CONFIG_HAVE_LONG_LONG` does not mean "use long long". I mean that it would be good to have an option to disable long long support if code needs to be tuned for small size even if compiler supports it. This is definitely not a scope of this PR, but do you consider this reasonable?
--
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] xiaoxiang781216 commented on pull request #6606: libc/stdio: enable long long formating by CONFIG_HAVE_LONG_LONG
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6606:
URL: https://github.com/apache/incubator-nuttx/pull/6606#issuecomment-1183967732
> @ALTracer Yes and that is exactly what I was posting in my comment above. Having decoupled `LIBC_LONG_LONG` and `HAVE_LONG_LONG` separately does not make sense because:
>
> ```
> #ifdef CONFIG_HAVE_LONG_LONG
> mount_sprintf(info, " %-10s %6llu%c %8llu%c %8llu%c %s\n", fstype,
> size, sizelabel, used, usedlabel, free, freelabel,
> mountpoint);
> #else
> mount_sprintf(info, " %-10s %6ld%c %8ld%c %8ld%c %s\n", fstype,
> size, sizelabel, used, usedlabel, free, freelabel,
> mountpoint);
> #endif
> ```
But this type of change isn't good, since we need modify all places which use long long. The better fix is let printf always support `ll` but cast to unsigned long like this:
```
unsigned long xxx = (unsigned long)var_arg(ap, unsigned long long);
```
In this case, the code size increase is very small.
--
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] acassis commented on pull request #6606: libc/stdio: enable long long formating by CONFIG_HAVE_LONG_LONG
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #6606:
URL: https://github.com/apache/incubator-nuttx/pull/6606#issuecomment-1183183761
I agree! I think even some NSH features like line editing and history should be enabled if DEFAULT_SMALL it not selected.
--
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] acassis merged pull request #6606: libc/stdio: enable long long formating by CONFIG_HAVE_LONG_LONG
Posted by GitBox <gi...@apache.org>.
acassis merged PR #6606:
URL: https://github.com/apache/incubator-nuttx/pull/6606
--
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] ALTracer commented on pull request #6606: libc/stdio: enable long long formating by CONFIG_HAVE_LONG_LONG
Posted by GitBox <gi...@apache.org>.
ALTracer commented on PR #6606:
URL: https://github.com/apache/incubator-nuttx/pull/6606#issuecomment-1183580273
I have a problem with this PR.
1. It contradicts a key philosophy of NuttX being small/lightweight and configurable.
2. It binds LIBC_LONG_LONG and HAVE_LONG_LONG together, but these options existed separately for a reason.
3. Forced code size increase of ~600* bytes, even though most printf invocations don't require ull handling.
4. No obvious way to disable HAVE_LONG_LONG (located solely in https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/compiler.h#L248)
My original PR #6605 aimed at a different problem and fixed it precisely. I never asked for system-wide breaking changes.
*TBD, see my later comments in #5253
I would like you to provide a way for users to either disable HAVE_LONG_LONG via Kconfig, or to introduce a separate option to forbid using long long division systemwide as well.
--
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