Re: [lmi] [lmi-commits] master bbcdaf5 1/2: Update commentary

[Vadim Zeitlin]

On Tue, 5 Jun 2018 03:17:21 +0000 Greg Chicares <address@hidden> wrote:

GC> How can we decide whether a char should be unsigned?

 AFAIK the answer to this question is pretty well established: it should be
unsigned iff it's not processed as a character.

GC> I would declare it unsigned iff we apply bit-wise operations to it. All
GC> we do with this variable is convert it to uppercase and compare it to
GC> {A,D,S}.

 Sorry, I forgot that we applied toupper() to it. This arguably does mean
that it's handled as a character, so maybe my initial objection was
misplaced. OTOH I still have a feeling that perhaps it's using toupper()
which is not ideal...

GC> I'd happily agree that this entity is an '8-bit integer' or a 'byte',

 FWIW now that we use C++17, we could use std::byte.

GC> but if we call it a 'char' of some sort, I don't see how we can say it's
GC> signed, unsigned, or signless. Maybe I should just change the first
GC> comment thus:
GC> 
GC> -///   3    1-byte char   :  Table type: {A, D, S} --> {age, duration, 
select}
GC> +///   3    1-byte integer:  Table type: {A, D, S} --> {age, duration, 
select}
GC> 
GC> For wider fields, we use std::int32_t and std::int16_t; following that
GC> pattern, this byte would be int8_t (i.e., a signed type).

 Yes, I think that this is a better fit than "char" here.


GC> But that's just nomenclature; I have a substantive concern in this file:
GC> 
GC>     static_assert(8 == CHAR_BIT);
GC>     static_assert(4 == sizeof(int));
GC>     static_assert(2 == sizeof(short int));
GC> 
GC> The "short int" assertion is needless because this TU doesn't use that
GC> type.

 I think it could be a leftover from the days before I switched to using
std::int16_t. In any case, it could, and should, indeed be removed, thanks
for noticing this.

GC> It's worrisome, though, because it has never yet been observed to
GC> throw, yet elsewhere we have:
GC>   typedef unsigned short int md5_uint32;
GC> and 32/8 is 4, which seems to contradict the assertion.

 This was already addressed in the next message in this thread.

GC> The "CHAR_BIT" assertion seems unlikely to be violated, unless we try
GC> porting to some DSP; yet POSIX requires a char to be an octet.

 Yes, it could well be redundant, but OTOH it doesn't really cost anything
to keep it.

GC> The middle "int" assertion would fail on an ILP64 machine:

 It's also unnecessary, and probably for the same reasons as the one
checking sizeof(short) above: the code now uses std::int32_t explicitly and
we only rely on int being larger than that and not on its exact size.

GC> What should we do for JDN?
GC>  - specify 'long int', so that the standard guarantees correctness?
GC>  - specify 'int' and assert that it's a 32-bit type?

 I think the latter, but assert that it's _at_least_ a 32-bit type, as
there should be no harm in it being larger.

GC> Furthermore, in 'census_view.cpp' we have
GC>   variant = static_cast<long int>(1 + row);
GC> and it does make sense to allow more than 32767 rows, or even 65535,
GC> but
GC>   unsigned int wxDataViewListCtrl::GetItemCount()
GC> seems to constrain that to sizeof(int) (which in practice means 2^32),
GC> so I guess we should use plain 'int' there. And we also have
GC>   wxSpinCtrl(...long int min, long int max, long int value);
GC> which AFAICT should be changed to plain int to match the wx API.

 The confusion between int and long in wxWidgets is, I'm afraid,
practically unfixable. They're used more or less interchangeably just about
everywhere and I just resigned myself to living with it a long time ago...
Practically speaking, I think "int" should be used everywhere and int64_t
whenever a larger range is required. I don't really see any use for long
nowadays, in wxWidgets case it's just a leftover from Win16 days.

 Regards,
VZ