[Podofo-users] PdfVariant: m_eDataType explained & Unknown sentinel value added
Hi zyx, hello all, > On 24 January 2019 at 18:55 zyx wrote: > > > Hi, ... snip ... > > > @zyx: Are you still vetoing that change of Francesco's (making > > Unknown equal to 0xff in EPdfDataType)? > > My complain from the past was about the compiler warnings (as Francesco > pointed out in the archives). The two changes together (r1959 and the > proposal) will make it work flawlessly, without the warnings, thus my > original objection is gone. In other words, just do it. Thank you very much :-). I'm so relieved because it didn't read so clearly before. > > > If not, I'll commit it after the fixes to the known security issues. > > Why to postpone? Why to split related commits by other commits? Originally because I planned to prioritize the security fixes. > Consider the r1959 caused this lengthy discussion. It would be more > than logic to close the discussion in the next revision, in r1960, not > to have some gap and only then finish something ongoing, while it's You've persuaded me, I have done the commit and it has indeed landed in svn r1960 [1] so that ends this discussion. > quite easy to forget of it. The changes I think of are: a) Unknown to > be 0xff; b) add comment above the change in r1959 explaining why it is > so. I would do a) and b) as one commit. The sooner you finish this In general I'd have treated them as two commits, because they don't depend on each other, but here, the elegance of "one finishing commit" swayed me. > two-liner the better. Now it's seven lines (with typo fixes and some reformatting as there is no 80-character limit followed in PdfVariant.h) ... Michal's assertion about space for a 32-bit enum is for another time, IMO (in the other thread originally about [PATCH 2/5]). > Bye, > zyx > Best regards, mabri ___ Podofo-users mailing list Podofo-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/podofo-users
Re: [Podofo-users] [PATCH 1/5] PdfVariant: m_eDataType is really a EPdfDataType
Hi, On Thu, 2019-01-24 at 17:43 +0100, Matthew Brincke wrote: > It isn't the case that Michal wrote they were the same, he only wrote > they are stored the same yes, I know. My test-in-action showed that what is stored and how it works are two different things. To make it bullet-proof the code needs to work the same in both cases (storage and comparison). And it's not the case. That's all I wanted to show. > @zyx: Are you still vetoing that change of Francesco's (making > Unknown equal to 0xff in EPdfDataType)? My complain from the past was about the compiler warnings (as Francesco pointed out in the archives). The two changes together (r1959 and the proposal) will make it work flawlessly, without the warnings, thus my original objection is gone. In other words, just do it. > If not, I'll commit it after the fixes to the known security issues. Why to postpone? Why to split related commits by other commits? Consider the r1959 caused this lengthy discussion. It would be more than logic to close the discussion in the next revision, in r1960, not to have some gap and only then finish something ongoing, while it's quite easy to forget of it. The changes I think of are: a) Unknown to be 0xff; b) add comment above the change in r1959 explaining why it is so. I would do a) and b) as one commit. The sooner you finish this two-liner the better. Bye, zyx ___ Podofo-users mailing list Podofo-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/podofo-users
Re: [Podofo-users] [PATCH 1/5] PdfVariant: m_eDataType is really a EPdfDataType
Hi zyx, hello all,
> On 24 January 2019 at 12:13 zyx wrote:
>
>
> On Wed, 2019-01-23 at 20:09 +0100, Michal Sudolsky wrote:
> > I am tempted to note that -1 is stored in 8-bit integer in exactly
> > same way as 0xFF.
>
it seems to me (after reading the remainder of zyx' post) that I should
have replied to the above already, emphasizing that Michal's statement
(before your post, I've begun this before Michal's answer of 14:52 UTC)
only holds with 8-bit integers (not enum values, which are usually 32-bit
except when they are "typesafe enums" of C++11 and later and explicitly
given a different width) and it only says they're stored the same way,
not that they compare the same (or match in a switch statement the same,
I just didn't think of you, zyx, using one).
> Hi,
> I used the code below with gcc 8.1.1 with this result (the result will
> make sense when the code is read):
>
>[0] is -1; same:0/1/0 as signed:-1/255 as unsigned:4294967295/255 switch
> match: s:1 u:0
>[1] is 0; same:1/1/1 as signed:0/0 as unsigned:0/0 switch match: s:1 u:1
>[2] is 1; same:1/1/1 as signed:1/1 as unsigned:1/1 switch match: s:1 u:1
>[3] is 255; same:0/0/1 as signed:-1/255 as unsigned:4294967295/255 switch
> match: s:0 u:1
>
> what it should write is to have the 'same' triple all ones in at least
> one column, not any of them zeros. You can see that 0xff and -1 are not
> the same when it comes to comparison, either with a real enum value or
> with signed and unsigned integers. I believe (according to my
It isn't the case that Michal wrote they were the same, he only wrote they
are stored the same (AIUI, reinterpret_cast(pdf_int8(-1)) ==
pdf_uint8(0xff) with PoDoFo integral types, IIRC, this specific syntax
may require C++11 for the initializers).
> experience) that enums are like *signed* integers. Thus one might be
> careful what replacement type is used and which values will be assigned
> to it.
An enum is "like" a signed integer if there are negative enum values in it
[1] and only then, except in the following meaning: being convertible to int
(which is signed [2]), if there aren't values larger than int's maximum value
in the enum type [3]).
>
> The change mabri committed doesn't fix anything to upstream, it fixes a
> thing only to Francesco's private (and modified) checkout. The change
> also doesn't break anything upstream. Unless, some time later, someone
@zyx: Are you still vetoing that change of Francesco's (making Unknown
equal to 0xff in EPdfDataType)? If not, I'll commit it after the fixes
to the known security issues. This may help deciding: it's already
unspecified, and from C++17 undefined behaviour, to assign a value
outside its range to an enum (range means the set of values represent-
able by the smallest bitfield the existing enum values fit in) [4].
> decides to make Unknown -1 or anything like that. Then, hopefully, the
> compiler will claim something, thus it'll be noticed.
I certainly am opposed to negative values in enum types (because I don't
enumerate with negatives), especially that one (because it's free of them).
Therefore I'm upholding my -1 against such a change.
> Bye,
> zyx
Best regards, mabri
>
> The used code follows. It generates compiler warnings with the switch-
> es, which is for good when looking for mistakes:
>
> /* g++ test.cpp -o test && ./test */
>
> #include
>
> typedef enum {
> enMinusOne = -1,
> enZero = 0,
> enOne = 1,
> enFF = 0xFF
> } EN;
>
> int
> main (void)
> {
> signed char n_int8;
> unsigned char n_uint8;
> bool smatch, umatch;
> int ii;
> EN ens[4] = { enMinusOne, enZero, enOne, enFF };
>
> for (ii = 0; ii < 4; ii++) {
> n_int8 = ens[ii];
> n_uint8 = ens[ii];
>
> #define test_switch(_val,_res) \
> switch (_val) { \
> case enMinusOne: _res = ii == 0; break; \
> case enZero: _res = ii == 1; break; \
> case enOne: _res = ii == 2; break; \
> case enFF: _res = ii == 3; break; \
> default: _res = false; break; \
> }
>
> test_switch (n_int8, smatch);
> test_switch (n_uint8, umatch);
>
> #undef test_switch
>
> printf ("[%d] is %d; same:%d/%d/%d as signed:%d/%d as
> unsigned:%u/%u switch match: s:%d u:%d\n", ii, ens[ii],
> n_int8 == n_uint8, n_int8 == ens[ii], n_uint8 ==
> ens[ii],
> n_int8, n_uint8, n_int8, n_uint8,
> smatch, umatch);
> }
>
> return 0;
> }
>
[1] https://en.cppreference.com/w/cpp/language/enum and there in the section
"Unscoped enumeration" under 1): "Declares an unscoped enumeration type
whose underlying type is not fixed (in this case, the underlying type is an
Re: [Podofo-users] [PATCH 1/5] PdfVariant: m_eDataType is really a EPdfDataType
On Thu, Jan 24, 2019 at 12:14 PM zyx wrote:
> On Wed, 2019-01-23 at 20:09 +0100, Michal Sudolsky wrote:
> > I am tempted to note that -1 is stored in 8-bit integer in exactly
> > same way as 0xFF.
>
> Hi,
> I used the code below with gcc 8.1.1 with this result (the result will
> make sense when the code is read):
>
>[0] is -1; same:0/1/0 as signed:-1/255 as unsigned:4294967295/255
> switch match: s:1 u:0
>[1] is 0; same:1/1/1 as signed:0/0 as unsigned:0/0 switch match: s:1 u:1
>[2] is 1; same:1/1/1 as signed:1/1 as unsigned:1/1 switch match: s:1 u:1
>[3] is 255; same:0/0/1 as signed:-1/255 as unsigned:4294967295/255
> switch match: s:0 u:1
>
> what it should write is to have the 'same' triple all ones in at least
> one column, not any of them zeros. You can see that 0xff and -1 are not
> the same when it comes to comparison, either with a real enum value or
> with signed and unsigned integers. I believe (according to my
> experience) that enums are like *signed* integers. Thus one might be
> careful what replacement type is used and which values will be assigned
> to it.
>
Unsigned char is zero-extended and signed char is sign-extended for
comparisons in your code sample. What would be seen from switch is that
when is stored either enMinusOne or enFF in n_int8 then it will be always
equal to enMinusOne and when stored in n_uint8 then enFF. So if there would
be None=-1 or Unknown=0xFF stored in 8-bit integer then later it cannot be
examined which one it was.
>
> The change mabri committed doesn't fix anything to upstream, it fixes a
> thing only to Francesco's private (and modified) checkout. The change
> also doesn't break anything upstream. Unless, some time later, someone
> decides to make Unknown -1 or anything like that. Then, hopefully, the
> compiler will claim something, thus it'll be noticed.
> Bye,
> zyx
>
> The used code follows. It generates compiler warnings with the switch-
> es, which is for good when looking for mistakes:
>
> /* g++ test.cpp -o test && ./test */
>
> #include
>
> typedef enum {
> enMinusOne = -1,
> enZero = 0,
> enOne = 1,
> enFF = 0xFF
> } EN;
>
> int
> main (void)
> {
> signed char n_int8;
> unsigned char n_uint8;
> bool smatch, umatch;
> int ii;
> EN ens[4] = { enMinusOne, enZero, enOne, enFF };
>
> for (ii = 0; ii < 4; ii++) {
> n_int8 = ens[ii];
> n_uint8 = ens[ii];
>
> #define test_switch(_val,_res) \
> switch (_val) { \
> case enMinusOne: _res = ii == 0; break; \
> case enZero: _res = ii == 1; break; \
> case enOne: _res = ii == 2; break; \
> case enFF: _res = ii == 3; break; \
> default: _res = false; break; \
> }
>
> test_switch (n_int8, smatch);
> test_switch (n_uint8, umatch);
>
> #undef test_switch
>
> printf ("[%d] is %d; same:%d/%d/%d as signed:%d/%d as
> unsigned:%u/%u switch match: s:%d u:%d\n", ii, ens[ii],
> n_int8 == n_uint8, n_int8 == ens[ii], n_uint8 ==
> ens[ii],
> n_int8, n_uint8, n_int8, n_uint8,
> smatch, umatch);
> }
>
> return 0;
> }
>
>
>
>
> ___
> Podofo-users mailing list
> Podofo-users@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/podofo-users
>
___
Podofo-users mailing list
Podofo-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/podofo-users
Re: [Podofo-users] [PATCH 1/5] PdfVariant: m_eDataType is really a EPdfDataType
On Wed, 2019-01-23 at 20:09 +0100, Michal Sudolsky wrote:
> I am tempted to note that -1 is stored in 8-bit integer in exactly
> same way as 0xFF.
Hi,
I used the code below with gcc 8.1.1 with this result (the result will
make sense when the code is read):
[0] is -1; same:0/1/0 as signed:-1/255 as unsigned:4294967295/255 switch
match: s:1 u:0
[1] is 0; same:1/1/1 as signed:0/0 as unsigned:0/0 switch match: s:1 u:1
[2] is 1; same:1/1/1 as signed:1/1 as unsigned:1/1 switch match: s:1 u:1
[3] is 255; same:0/0/1 as signed:-1/255 as unsigned:4294967295/255 switch
match: s:0 u:1
what it should write is to have the 'same' triple all ones in at least
one column, not any of them zeros. You can see that 0xff and -1 are not
the same when it comes to comparison, either with a real enum value or
with signed and unsigned integers. I believe (according to my
experience) that enums are like *signed* integers. Thus one might be
careful what replacement type is used and which values will be assigned
to it.
The change mabri committed doesn't fix anything to upstream, it fixes a
thing only to Francesco's private (and modified) checkout. The change
also doesn't break anything upstream. Unless, some time later, someone
decides to make Unknown -1 or anything like that. Then, hopefully, the
compiler will claim something, thus it'll be noticed.
Bye,
zyx
The used code follows. It generates compiler warnings with the switch-
es, which is for good when looking for mistakes:
/* g++ test.cpp -o test && ./test */
#include
typedef enum {
enMinusOne = -1,
enZero = 0,
enOne = 1,
enFF = 0xFF
} EN;
int
main (void)
{
signed char n_int8;
unsigned char n_uint8;
bool smatch, umatch;
int ii;
EN ens[4] = { enMinusOne, enZero, enOne, enFF };
for (ii = 0; ii < 4; ii++) {
n_int8 = ens[ii];
n_uint8 = ens[ii];
#define test_switch(_val,_res) \
switch (_val) { \
case enMinusOne: _res = ii == 0; break; \
case enZero: _res = ii == 1; break; \
case enOne: _res = ii == 2; break; \
case enFF: _res = ii == 3; break; \
default: _res = false; break; \
}
test_switch (n_int8, smatch);
test_switch (n_uint8, umatch);
#undef test_switch
printf ("[%d] is %d; same:%d/%d/%d as signed:%d/%d as
unsigned:%u/%u switch match: s:%d u:%d\n", ii, ens[ii],
n_int8 == n_uint8, n_int8 == ens[ii], n_uint8 ==
ens[ii],
n_int8, n_uint8, n_int8, n_uint8,
smatch, umatch);
}
return 0;
}
___
Podofo-users mailing list
Podofo-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/podofo-users
