[PATCH] Remove workarounds to format [u]int64's

пн, 21 мар. 2022 г. в 12:52, Aleksander Alekseev <aleksander@timescale.com>:

Hi hackers,

I learned from Tom [1] that we can simplify the code like:

```
char buff[32];
snprintf(buf, sizeof(buf), INT64_FORMAT, ...)
ereport(WARNING, (errmsg("%s ...", buf)));
```

... and rely on %lld/%llu now as long as we explicitly cast the
argument to long long int / unsigned long long. This was previously
addressed in 6a1cd8b9 and d914eb34, but I see more places where we
still use an old approach.

Suggested patch fixes this. Tested locally - no warnings; passes all the
tests.

[1]
/messages/by-id/771048.1647528068@sss.pgh.pa.us

Hi, Alexander!

Probably you can do (long long) instead of (long long int). It is shorter
and this is used elsewhere in the code.

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

Hi Pavel,

Probably you can do (long long) instead of (long long int). It is shorter and this is used elsewhere in the code.

Thanks! Here is the updated patch. I also added Reviewed-by: and
Discussion: to the commit message.

--
Best regards,
Aleksander Alekseev

Probably you can do (long long) instead of (long long int). It is

shorter and this is used elsewhere in the code.

Thanks! Here is the updated patch. I also added Reviewed-by: and
Discussion: to the commit message.

Thanks, Alexander!
I suggest the patch is in a good shape to be committed.
(
Maybe some strings that don't fit screen cloud be reflowed:
(long long int)seqdataform->last_value, (long long int)seqform->seqmax)));
)

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

On Mon, 21 Mar 2022 at 17:23, Aleksander Alekseev <aleksander@timescale.com> wrote:

Hi Pavel,

Probably you can do (long long) instead of (long long int). It is shorter and this is used elsewhere in the code.

Thanks! Here is the updated patch. I also added Reviewed-by: and
Discussion: to the commit message.

Hi,

After apply the patch, I found pg_checksums.c also has the similar code.

In progress_report(), I'm not sure we can do this replace for this code.

snprintf(total_size_str, sizeof(total_size_str), INT64_FORMAT,
total_size / (1024 * 1024));
snprintf(current_size_str, sizeof(current_size_str), INT64_FORMAT,
current_size / (1024 * 1024));

fprintf(stderr, _("%*s/%s MB (%d%%) computed"),
(int) strlen(current_size_str), current_size_str, total_size_str,
percent);

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Hi Japin,

After apply the patch, I found pg_checksums.c also has the similar code.

Thanks for noticing it.

In progress_report(), I'm not sure we can do this replace for this code.

I added the corresponding change as a separate commit so it can be
easily reverted if necessary.

Here is a complete patchset with some additional changes by me.

--
Best regards,
Aleksander Alekseev

Hi Japin,

As Tom said in [1], we don't need to touch the *.po files, since those

files

are managed by the translation team.

[1]

/messages/by-id/1110708.1647623560@sss.pgh.pa.us

True, but I figured that simplifying the work of the translation team would
not harm either. In any case, the committer can easily exclude these
changes from the patch, if necessary.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@timescale.com> writes:

As Tom said in [1], we don't need to touch the *.po files, since those
files are managed by the translation team.

True, but I figured that simplifying the work of the translation team would
not harm either.

It would not simplify things for them at all, just mess it up.
The master copies of the .po files are kept in a different repo.
Also, I believe that extraction of new message strings is automated
already.

https://www.postgresql.org/docs/devel/nls.html

https://wiki.postgresql.org/wiki/NLS

regards, tom lane

Hi Tom,

It would not simplify things for them at all, just mess it up.
The master copies of the .po files are kept in a different repo.
Also, I believe that extraction of new message strings is automated
already.

Got it, thanks. Here is the corrected patch. It includes all the
changes by me and Japin, and doesn't touch PO files.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@timescale.com> writes:

Got it, thanks. Here is the corrected patch. It includes all the
changes by me and Japin, and doesn't touch PO files.

Pushed. I removed now-unnecessary braces, reflowed some lines
as suggested by Pavel, and pgindent'ed (which insisted on adding
spaces after the casts, as is project style).

regards, tom lane

On 21.03.22 15:37, Aleksander Alekseev wrote:

It would not simplify things for them at all, just mess it up.
The master copies of the .po files are kept in a different repo.
Also, I believe that extraction of new message strings is automated
already.

Got it, thanks. Here is the corrected patch. It includes all the
changes by me and Japin, and doesn't touch PO files.

I think in some cases we can make this even simpler (and cast-free) by
changing the underlying variable to be long long instead of int64.
Especially in cases where the whole point of the variable is to be some
counter that ends up being printed, there isn't a need to use int64 in
the first place. See attached patch for examples.

On 21.03.22 15:37, Aleksander Alekseev wrote:

It would not simplify things for them at all, just mess it up.
The master copies of the .po files are kept in a different repo.
Also, I believe that extraction of new message strings is automated
already.

Got it, thanks. Here is the corrected patch. It includes all the
changes by me and Japin, and doesn't touch PO files.

I think in some cases we can make this even simpler (and cast-free) by
changing the underlying variable to be long long instead of int64.
Especially in cases where the whole point of the variable is to be some
counter that ends up being printed, there isn't a need to use int64 in
the first place. See attached patch for examples.

Yes, this will work, when we can define a variable itself as *long long*.
But for some applications: [1]/messages/by-id/CACG=ezYV3FM5i9ws2QLyF+rz5WHTqheL59VRsHGsgAwfx8gh4g@mail.gmail.com, [2]/messages/by-id/CACG=ezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe=pyyjVWA@mail.gmail.com, I suppose we'll need exactly uint64 to
represent TransactionId. uint64 is warrantied to fit into *unsigned long
long*, but on most of archs it is just *unsigned long*. Defining what we
need to be uint64 as *unsigned long long* on these archs will mean it
become uint128, which we may not like.

In my opinion, in many places, it's better to have casts when it's for
printing fixed-width int/uint variables than the alternative.

[1]: /messages/by-id/CACG=ezYV3FM5i9ws2QLyF+rz5WHTqheL59VRsHGsgAwfx8gh4g@mail.gmail.com
/messages/by-id/CACG=ezYV3FM5i9ws2QLyF+rz5WHTqheL59VRsHGsgAwfx8gh4g@mail.gmail.com
[2]: /messages/by-id/CACG=ezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe=pyyjVWA@mail.gmail.com
/messages/by-id/CACG=ezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe=pyyjVWA@mail.gmail.com

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;