From: Albert ARIBAUD <albert.aribaud@3adev.fr>
To: Paul Eggert <eggert@cs.ucla.edu>
Cc: libc-alpha@sourceware.org
Subject: Re: [PATCH v5 1/2] Y2038: Add 64-bit time for all architectures
Date: Wed, 20 Jun 2018 16:46:00 -0000 [thread overview]
Message-ID: <20180620184553.727f1e6f@athena> (raw)
In-Reply-To: <cb7ddf2c-6871-7f46-2fd5-241b9ac7cb62@cs.ucla.edu>
Hi Paul,
On Wed, 20 Jun 2018 09:01:08 -0700, Paul Eggert <eggert@cs.ucla.edu>
wrote :
> Albert ARIBAUD wrote:
>
> > Regarding the cast, there is no way to reduce the /need/ for casts, as
> > we /do/ need one here. What we can do is reduce the number of explicit
> > casts
>
> A terminology point: the C Standard uses the word "cast" to describe what you're
> calling an "explicit cast", and it uses the word "conversion" to describe what
> you're calling a "cast". Let's stick to the standard terminology as it makes for
> less confusion.
Agreed.
> > But I disagree that the resulting code would be as clear as the one in
> > the patch: it would in fact be less clear, because the intent of the
> > code would become implicit rather than explicit in two places:
>
> First, in C, casts are more dangerous than other conversions because they allow
> more typos to go unchecked. If you mistakenly cast an integer to a pointer, the
> compiler often does not complain, but if you mistakenly convert an integer to a
> pointer without casting it, the compiler will catch your mistake and report an
> error. For this reason, C casts should not be used unless necessary (and they
> are not necessary here).
Indeed when the cast is /created/, there is a risk that it be wrong.
But once it is created and reviewed and found correct, then in a
function that risk is gone for good IMO since both the type being cast
to and the type being cast from are known -- of course, I would stand
firm against any cast being done to a macro argument, as you cannot
tell what type it might have, if it has any.
> Second, the code I proposed is completely obvious. One cannot read code like this:
>
> type1 x = ...;
> type2 y = x;
> if (y == x) ...
>
> without immediately knowing what's going on.
Here, I would beg to differ on what one can or cannot immediately know
what's going on without explicit clues. In my experience, what one
immediately knows from a piece of code varies wildly across
individuals, and Murphy is extremely efficient in ensuring that what
goes without saying always ends up going wrong at some point.
> Third, as you noted, the proposed fits_in_time_t function does a poor job of
> moving common code into a single function. To do a better job we would need
> something like the reduce_to_time_t function of your email. Unfortunately, as
> you noted in a later email, reduce_to_time_t doesn't work because of include
> problems. The exact same thought process went through my head when I wrote my
> review. That is, I thought "fits_in_time_t is a bad helper function, and trying
> to improve it by having a helper function that captures the actual idea won't
> work due to include hassles, so let's just do things directly; it's just as
> clear and it solves the problem better".
>
> So, let's just do things directly; it's just as clear and it solves the problem
> better.
There's the possibility of replacing function reduce_to_time_t() with a
macro, which would postpone evaluation of __set_errno() until within
the calling C file, but then, a cast in a macro is a worse idea than a
cast in a function.
I still dislike the idea of duplicating code, but here specifically, I
can't see another way without doing bigger and deeper changes than I
need to. I also still don't think the cast discussed here would be a
risk or bad practice, but I'm ok with removing it provided the readers
get a stronger hint on what's going on unsaid. So I'll go with the
code you suggest, with a more explicit name for the temporary and a
one-line short comment.
/* Convert from __time64 to time_t or fail with EOVERFLOW. */
time_t t32 = t64;
if (t32 == t64)
return t32;
__set_errno (EOVERFLOW);
return -1;
Cordialement,
Albert ARIBAUD
3ADEV
next prev parent reply other threads:[~2018-06-20 16:46 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-18 19:14 [PATCH v5 0/2] Y2038 support batch 1 - __time64_t and __tz_convert Albert ARIBAUD (3ADEV)
2018-06-18 19:14 ` [PATCH v5 2/2] Y2038: make __tz_convert compatible with 64-bit-time Albert ARIBAUD (3ADEV)
2018-06-19 23:35 ` Paul Eggert
2018-06-20 6:06 ` Albert ARIBAUD
2018-06-18 19:14 ` [PATCH v5 1/2] Y2038: Add 64-bit time for all architectures Albert ARIBAUD (3ADEV)
2018-06-19 23:25 ` Paul Eggert
2018-06-20 6:04 ` Albert ARIBAUD
2018-06-20 6:29 ` Albert ARIBAUD
2018-06-20 7:16 ` Albert ARIBAUD
2018-06-20 16:01 ` Paul Eggert
2018-06-20 16:46 ` Albert ARIBAUD [this message]
2018-06-20 17:50 ` Paul Eggert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180620184553.727f1e6f@athena \
--to=albert.aribaud@3adev.fr \
--cc=eggert@cs.ucla.edu \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).