[vim/vim] Format positional arguments (PR #12140)
cvwillegen
This PR adds the possibility to use positional arguments in the printf() function, and in translation files, opening the possibility for translators to specify word order in translations.
Ex: echo printf("My name is %2$s, %1$s", "Bram", "Moolenaar")
echo printf("My date of birth is: %03$*1$d/%04$*1$d/%05$*2$d", 2, 4, 13, 7, 2002)
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/12140
Commit Summary
- 8faa037 Support positional arguments in string formatting
- c4abeda Try to fix build errors.
- 7ecd3aa Move type strings to globals.h
- 0ce79fb Fix message declarations
- a27ec4d Fix GCC warnings about invalid format strings
- 710a0ba Fix tab/space mixup.
- 692612b Merge branch 'vim:master' into format-positional-arguments
File Changes
(8 files)
- M runtime/doc/builtin.txt (94)
- M src/errors.h (12)
- M src/globals.h (13)
- M src/message_test.c (110)
- M src/po/check.vim (6)
- M src/strings.c (831)
- M src/testdir/Make_all.mak (1)
- A src/testdir/test_format.vim (295)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
cvwillegen
Fixes #10577
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 08c9923 Merge branch 'vim:master' into format-positional-arguments
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #12140 (08c9923) into master (d13dd30) will decrease coverage by
81.57%.
The diff coverage is45.50%.
@@ Coverage Diff @@ ## master #12140 +/- ## =========================================== - Coverage 81.94% 0.38% -81.57% =========================================== Files 164 154 -10 Lines 194094 179629 -14465 Branches 43829 41033 -2796 =========================================== - Hits 159056 685 -158371 - Misses 22204 178853 +156649 + Partials 12834 91 -12743
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | ? |
|
| huge-gcc-none | ? |
|
| huge-gcc-testgui | ? |
|
| huge-gcc-unittests | 0.38% <45.50%> (+0.09%) |
⬆️ |
| linux | 0.38% <45.50%> (-82.00%) |
⬇️ |
| mingw-x64-HUGE | ? |
|
| mingw-x86-HUGE | ? |
|
| windows | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/strings.c | 23.50% <43.18%> (-69.26%) |
⬇️ |
| src/message_test.c | 100.00% <100.00%> (ø) |
... and 157 files with indirect coverage changes
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 364fcbc Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 2 commits.
- 8a6a008 Improve test coverage, fix uncovered errors.
- f960924 Merge branch 'format-positional-arguments' of github.com:cvwillegen/vim into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 14fc41e Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 40c0e7d Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 03d9209 Give E767 for positional printf() call w/ too many params
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- a0aa99a Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- bc5b502 Fix order of flags and positional arguments.
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 75d00c9 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 2 commits.
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 1ecf570 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 9d7520b Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 0750e4b Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 4f8b44d Improve test coverage, check error message paths.
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 7c01be0 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
Christ van Willegen
cvwillegen
@cvwillegen pushed 4 commits.
- 1f76138 patch 9.0.1427: warning for uninitialized variable
- c174c2e patch 9.0.1428: cursor in wrong position when leaving insert mode
- 1a08a3e patch 9.0.1429: invalid memory access when ending insert mode
- 67123a5 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 5537c94 Add more test cases.
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- ba410d7 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- f5bb2fd Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 2360c42 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- dc9947d Merge branch 'master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- d887a19 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- abad145 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- c2d9bd3 Fix code style.
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 0b78545 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
Restorer
This PR adds the possibility to use positional arguments in the printf() function, and in translation files, opening the possibility for translators to specify word order in translations.
@cvwillegen
If I understood you correctly, then the built-in gettext tools can control the order of arguments during translation.
Described here in this section of the gettext documentation.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
cvwillegen
On Tue, May 23, 2023 at 9:21 PM Restorer ***@***.***> wrote:
> @cvwillegen
> If I understood you correctly, then the built-in gettext tools can control the order of arguments during translation.
> Described here in this section of the gettext documentation.
runtime needs to be able to format strings that are written that way,
and it wasn't (isn't) able to. This PR adds that functionality so that
translators are more free to translate to correct word order.
Christ van Willegen
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Restorer
@cvwillegen
That is, directly in the script files? Did I understand correctly?
If so, then Bram somehow promises to think about how to do this for third‐party scripts #11637 .
And for those distributed with Vim, he has already made a built‐in gettext() function and, accordingly, available mechanisms for PO files can be used for them.
But if you succeed in doing what you are doing, then it will be great.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- faf701f Merge branch 'vim:master' into format-positional-arguments
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
Can you please update and rebase this PR to the latest?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- cd002b4 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
***@***.***>:
> Can you please update and rebase this PR to the latest?
>
Christ
>
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
can someone please try this out?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 9d3ea28 Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
cvwillegen
@cvwillegen pushed 1 commit.
- 8eace5c Merge branch 'vim:master' into format-positional-arguments
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Restorer
@RestorerZ commented on this pull request.
In src/globals.h:
> @@ -1674,6 +1674,19 @@ EXTERN int cmdwin_result INIT(= 0); // result of cmdline window or 0
EXTERN char_u no_lines_msg[] INIT(= N_("--No lines in buffer--"));
+EXTERN char typename_unknown[] INIT(= N_("unknown"));
+EXTERN char typename_int[] INIT(= N_("int"));
+EXTERN char typename_longint[] INIT(= N_("long int"));
+EXTERN char typename_longlongint[] INIT(= N_("long long int"));
+EXTERN char typename_unsignedint[] INIT(= N_("unsigned int"));
I'm not sure if the format specifier type needs to be translated in the error message. Types of the C language are most often written int, long, etc.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@cvwillegen
This PR introduced 2 warnings in Coverity. Can you please have a look at it:
Hi,
Please find the latest report on new defect(s) introduced to vim found with Coverity Scan.
2 new defect(s) introduced to vim found with Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 2 of 2 defect(s)
** CID 1539915: API usage errors (VARARGS)
/src/strings.c: 2881 in skip_to_arg()
________________________________________________________________________________________________________
*** CID 1539915: API usage errors (VARARGS)
/src/strings.c: 2881 in skip_to_arg()
2875 // Because we know that after we return from this call,
2876 // a va_arg() call is made, we can pre-emptively
2877 // increment the current argument index.
2878 ++*arg_cur;
2879 ++*arg_idx;
2880
CID 1539915: API usage errors (VARARGS)
"va_end" was not called for "*ap".
2881 return;
2882 }
2883
2884 int
2885 vim_vsnprintf_typval(
2886 char *str,
** CID 1539914: (FORWARD_NULL)
________________________________________________________________________________________________________
*** CID 1539914: (FORWARD_NULL)
/src/strings.c: 3309 in vim_vsnprintf_typval()
3303 // signed
3304 switch (length_modifier)
3305 {
3306 case '\0':
3307 case 'h':
3308 // char and short arguments are passed as int.
CID 1539914: (FORWARD_NULL)
Passing null pointer "ap_types" to "skip_to_arg", which dereferences it.
3309 int_arg =
3310 # if defined(FEAT_EVAL)
3311 tvs != NULL ? tv_nr(tvs, &arg_idx) :
3312 # endif
3313 (skip_to_arg(ap_types, ap_start, &ap, &arg_idx, &arg_cur),
3314 va_arg(ap, int));
/src/strings.c: 3583 in vim_vsnprintf_typval()
3577 double f;
3578 double abs_f;
3579 char format[40];
3580 int l;
3581 int remove_trailing_zeroes = FALSE;
3582
CID 1539914: (FORWARD_NULL)
Passing null pointer "ap_types" to "skip_to_arg", which dereferences it.
3583 f =
3584 # if defined(FEAT_EVAL)
3585 tvs != NULL ? tv_float(tvs, &arg_idx) :
3586 # endif
3587 (skip_to_arg(ap_types, ap_start, &ap, &arg_idx, &arg_cur),
3588 va_arg(ap, double));
/src/strings.c: 3290 in vim_vsnprintf_typval()
3284
3285 if (ptr_arg != NULL)
3286 arg_sign = 1;
3287 }
3288 else if (fmt_spec == 'b' || fmt_spec == 'B')
3289 {
CID 1539914: (FORWARD_NULL)
Passing null pointer "ap_types" to "skip_to_arg", which dereferences it.
3290 bin_arg =
3291 # if defined(FEAT_EVAL)
3292 tvs != NULL ?
3293 (uvarnumber_T)tv_nr(tvs, &arg_idx) :
3294 # endif
3295 (skip_to_arg(ap_types, ap_start, &ap, &arg_idx, &arg_cur),
/src/strings.c: 3166 in vim_vsnprintf_typval()
3160 break;
3161
3162 case 'c':
3163 {
3164 int j;
3165
CID 1539914: (FORWARD_NULL)
Passing null pointer "ap_types" to "skip_to_arg", which dereferences it.
3166 j =
3167 # if defined(FEAT_EVAL)
3168 tvs != NULL ? tv_nr(tvs, &arg_idx) :
3169 # endif
3170 (skip_to_arg(ap_types, ap_start, &ap, &arg_idx, &arg_cur),
3171 va_arg(ap, int));
/src/strings.c: 3277 in vim_vsnprintf_typval()
3271 // conversion
3272 void *ptr_arg = NULL;
3273
3274 if (fmt_spec == 'p')
3275 {
3276 length_modifier = '\0';
CID 1539914: (FORWARD_NULL)
Passing null pointer "ap_types" to "skip_to_arg", which dereferences it.
3277 ptr_arg =
3278 # if defined(FEAT_EVAL)
3279 tvs != NULL ? (void *)tv_str(tvs, &arg_idx,
3280 NULL) :
3281 # endif
3282 (skip_to_arg(ap_types, ap_start, &ap, &arg_idx, &arg_cur),
/src/strings.c: 3033 in vim_vsnprintf_typval()
3027 uj = 10 * uj + (unsigned int)(*p++ - '0');
3028 arg_idx = uj;
3029
3030 ++p;
3031 }
3032
CID 1539914: (FORWARD_NULL)
Passing null pointer "ap_types" to "skip_to_arg", which dereferences it.
3033 j =
3034 # if defined(FEAT_EVAL)
3035 tvs != NULL ? tv_nr(tvs, &arg_idx) :
3036 # endif
3037 (skip_to_arg(ap_types, ap_start, &ap, &arg_idx, &arg_cur),
3038 va_arg(ap, int));
/src/strings.c: 3181 in vim_vsnprintf_typval()
3175 str_arg = (char *)&uchar_arg;
3176 break;
3177 }
3178
3179 case 's':
3180 case 'S':
CID 1539914: (FORWARD_NULL)
Passing null pointer "ap_types" to "skip_to_arg", which dereferences it.
3181 str_arg =
3182 # if defined(FEAT_EVAL)
3183 tvs != NULL ? tv_str(tvs, &arg_idx, &tofree) :
3184 # endif
3185 (skip_to_arg(ap_types, ap_start, &ap, &arg_idx, &arg_cur),
3186 va_arg(ap, char *));
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
cvwillegen
I will check for NULL in the function and make it an internal error, because this shouldn't happen.
The va_end() warning is not correct, because it is called at the end of the vim_vsprintf() function, and at the start of the skip_to_arg() function, so I made sure that all calls balance out. I'm not sure how to silence this warning?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/strings.c:
> + semsg(_(e_positional_nr_out_of_bounds_str), arg_idx + 1, fmt); + goto error; + } +# endif + } + + return OK; + +error: + vim_free(*ap_types); + *ap_types = NULL; + *num_posarg = 0; + return FAIL; +} + + void
This function is used only within this file, so this function can be changed to a static function.
In src/strings.c:
> + return FAIL;
+}
+
+ void
+skip_to_arg(
+ const char **ap_types,
+ va_list ap_start,
+ va_list *ap,
+ int *arg_idx,
+ int *arg_cur)
+{
+ int arg_min = 0;
+
+ if (*arg_cur + 1 == *arg_idx)
+ {
+ ++*arg_cur;
Can you add a comment here describing why this is needed (similar to the ones below)?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
I will check for NULL in the function and make it an internal error, because this shouldn't happen.
I think it can happen, but in that case it is never dereferenced.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
For example, it can happen when the first format specifier is %c.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@cvwillegen any updates here?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
cvwillegen
I'll get to it after my holiday...
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
sorry, no problem, please have your time. I just wanted not to get this lost.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
/remind me about it in 1 week
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
cvwillegen
For example, it can happen at the first format specifier.
I looked at the code again. My conclusion is:
When there are no positional arguments specified, ap_types remains NULL. ap_types is, indeed, dereferenced in skip_to_arg() without checking for NULL, but only if the argument being requested differs from the last one + 1, and that can only happen if there are positional arguments. So, the skipping code is not executed.
If ap_types is NULL when positional arguments are specified, then the parse_fmt_types() function has failed allocating memory somewhere and it returns FAIL, and in this case the rest of the code is not executed.
I did add an internal error on the dereferencing (should not happen), but that's basically to shut up the warning :-) If it does happen, at least we know the format string an index where this went wrong.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.