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
Re: [Podofo-users] [PATCH 1/5] PdfVariant: m_eDataType is really a EPdfDataType
> > > AFAIK, "None" and "Unknown" are different (e.g. for filters "None" means > it's known that no filter is applied whereas "Unknown" means it isn't > known which filter is applied (or the filter applied is unsupported in > PoDoFo, at least in the used revision). Therefore, where "None" makes > sense, I'm OK with (actually, would prefer) it being the first value, > but not -1 (or other negative) unless it'd be necessary to prevent zero- > initialization to yield "None" as valid value (on the contrary, I'd be > concerned if it'd yield some arbitrary, maybe unintended filter, even if > documented as the default filter, except in default construction > PdfFilter()). > > Another reason is that I wouldn't know, reading the source code, how many > 1- (set) bits I'd get with an enum value -1, given the enum size can change > by compiler option (with 0xff any extra bits would be zeroed padding > AFAICS). > > I am tempted to note that -1 is stored in 8-bit integer in exactly same way as 0xFF. > > why not? I kind of miss what you are aiming for. Is it anything else > > than having all Unknown being 0xff, which can eventually cause waste of > > memory in combination with -fshort-enums (while the switch is > > In the reference Francesco mentioned in his first answer [1] to my first > post "[PATCH 1/5] is problematic" I couldn't find anything suggesting the > existence of less-than-8-bit enums, so 0xff (can be stored in 8 bit) does > waste clearly no memory, even with -fshort-enums (I didn't search all its > pages, just read the one about that option, but IIRC types shorter than 8 > bit are only possible with bitfields). > > > > > > If you are reverting > > > > I'm not going to revert anything. I just express my opinion on the > > change. > > > > @zyx: In light of what I've written above (and in pots before), would you > please allow the change making Unknown = 0xff go in? > > > I will not complain but I kindly ask you to comment the code with a > > > warning so nobody will attempt to change that underlying type anymore > > > > Right, it makes sense to add the comment there. I'm not going to add it > > there myself though. Not yet at least. > > I'm volunteering to add it, but only after the security fixes are all in, > and the Unknown has been added with positive value. > > > > > Bye, > > zyx > > Best regards, Matthew > > [1] Message-ID: 1-o+bf4cejso2kyho0e-_00g2b7u7ree9...@mail.gmail.com> > Date: Tue, 22 Jan 2019 23:16:02 +0100 > Archive URL: > https://sourceforge.net/p/podofo/mailman/message/36524577/ > > > ___ > 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
Hi zyx, hello all, > On 23 January 2019 at 15:39 zyx wrote: > > > Hi, > > On Wed, 2019-01-23 at 13:35 +0100, Francesco Pretto wrote: > > > weird, neither my checkout has it, nor the trunk: > > > > I have ePdfDataType_Unknown = 0xff because I pushed for this change > > some time ago[1]. > > I see, that's a long time ago. > > > I ask you if we can move on with the change applied by Matthew, which > > I think is wise and has the benefit of accommodating the change I was > > aiming for. @Francesco: Thank you. > > I gave you a counter-example, where the committed change will break > things (when the Unknown would be changed to -1 instead). For the I wouldn't enumerate anything with a negative value, so IMO such a change is to be -1'd (at least I would not accept non-"natural numbers" in it). > consistency, the None is set to -1, the (other) Unknown-s to 0xff. Why > not make them consistent in a way to declare Unknown/None as the first > value of the enum with the value -1? Would that work for you? If not, AFAIK, "None" and "Unknown" are different (e.g. for filters "None" means it's known that no filter is applied whereas "Unknown" means it isn't known which filter is applied (or the filter applied is unsupported in PoDoFo, at least in the used revision). Therefore, where "None" makes sense, I'm OK with (actually, would prefer) it being the first value, but not -1 (or other negative) unless it'd be necessary to prevent zero- initialization to yield "None" as valid value (on the contrary, I'd be concerned if it'd yield some arbitrary, maybe unintended filter, even if documented as the default filter, except in default construction PdfFilter()). Another reason is that I wouldn't know, reading the source code, how many 1- (set) bits I'd get with an enum value -1, given the enum size can change by compiler option (with 0xff any extra bits would be zeroed padding AFAICS). > why not? I kind of miss what you are aiming for. Is it anything else > than having all Unknown being 0xff, which can eventually cause waste of > memory in combination with -fshort-enums (while the switch is In the reference Francesco mentioned in his first answer [1] to my first post "[PATCH 1/5] is problematic" I couldn't find anything suggesting the existence of less-than-8-bit enums, so 0xff (can be stored in 8 bit) does waste clearly no memory, even with -fshort-enums (I didn't search all its pages, just read the one about that option, but IIRC types shorter than 8 bit are only possible with bitfields). > > > If you are reverting > > I'm not going to revert anything. I just express my opinion on the > change. > @zyx: In light of what I've written above (and in pots before), would you please allow the change making Unknown = 0xff go in? > > I will not complain but I kindly ask you to comment the code with a > > warning so nobody will attempt to change that underlying type anymore > > Right, it makes sense to add the comment there. I'm not going to add it > there myself though. Not yet at least. I'm volunteering to add it, but only after the security fixes are all in, and the Unknown has been added with positive value. > > Bye, > zyx Best regards, Matthew [1] Message-ID: Date: Tue, 22 Jan 2019 23:16:02 +0100 Archive URL: https://sourceforge.net/p/podofo/mailman/message/36524577/ ___ 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, 23 Jan 2019 at 15:39, zyx wrote: > > I gave you a counter-example, where the committed change will break > things (when the Unknown would be changed to -1 instead). For the > consistency, the None is set to -1, the (other) Unknown-s to 0xff. Why > not make them consistent in a way to declare Unknown/None as the first > value of the enum with the value -1? Would that work for you? If not, > why not? I kind of miss what you are aiming for. Is it anything else > than having all Unknown being 0xff, which can eventually cause waste of > memory in combination with -fshort-enums (while the switch is > potentially dangerous, it can be useful for someone whom bundles PoDoFo > into his/her own project and doesn't expose it into the public)? > > All you say makes sense but I'm gasping on inconsistencies all around the library and I genuinely tried to follow the most used convention by putting Unknown on 0xff. Actually, I follow this personal "golden rule" for *my* enums: 0 : UNKNOWN or a enumeration value with a semantic that make sense in a default initialization (both fit very well with zero initialization) 1...x : Actual values of the enumeration to be specified voluntarily -1 : NONE or ANY: values with special meaning to be specified voluntarily Of course I am not pushing such change because it would require review of all the enums in the library. For EPdfDataType particular case I would also be fine to have Unknown = -1 but I didn't do it this way as said because I tried to follow what is done in other enums. ___ 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 Wed, 2019-01-23 at 13:35 +0100, Francesco Pretto wrote: > > weird, neither my checkout has it, nor the trunk: > > I have ePdfDataType_Unknown = 0xff because I pushed for this change > some time ago[1]. I see, that's a long time ago. > I ask you if we can move on with the change applied by Matthew, which > I think is wise and has the benefit of accommodating the change I was > aiming for. I gave you a counter-example, where the committed change will break things (when the Unknown would be changed to -1 instead). For the consistency, the None is set to -1, the (other) Unknown-s to 0xff. Why not make them consistent in a way to declare Unknown/None as the first value of the enum with the value -1? Would that work for you? If not, why not? I kind of miss what you are aiming for. Is it anything else than having all Unknown being 0xff, which can eventually cause waste of memory in combination with -fshort-enums (while the switch is potentially dangerous, it can be useful for someone whom bundles PoDoFo into his/her own project and doesn't expose it into the public)? > If you are reverting I'm not going to revert anything. I just express my opinion on the change. > I will not complain but I kindly ask you to comment the code with a > warning so nobody will attempt to change that underlying type anymore Right, it makes sense to add the comment there. I'm not going to add it there myself though. Not yet at least. 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
On Wed, 23 Jan 2019 at 13:47, Joerg Sonnenberger wrote: > This is intentionally not using the enum > directly as the enum would be larger. And this had been already clarified by Matthew in the other thread, which of course I have no objection. ___ 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, 23 Jan 2019 at 13:47, Joerg Sonnenberger wrote: > No, you are missing the point. This is intentionally not using the enum > directly as the enum would be larger. Using C++11 sized enums on the > other hand created funny ABI changes. > > Really, I'm not missing the point. There was no attempt in my patch to use C++11 sized enums (I just *said* that c++11 has a concept for controlling enum size. Also migrating to c++11 type safe enums would be API breaking, so of course I would not push this change). Eventually the change pushed by Matthew just changed signed -> unsigned for a private member, so ABI is not changing at all. ___ 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, Jan 23, 2019 at 12:28:59PM +0100, Francesco Pretto wrote: > On Wed, 23 Jan 2019 at 12:18, Joerg Sonnenberger wrote: > > > > On Mon, Jan 14, 2019 at 11:08:03PM +0100, Francesco Pretto wrote: > > > > > > From: Francesco Pretto > > > > > > Fixes warnings in linux since the maximum value of EPdfDataType is 255 > > > > Please don't do this and look at the history of the code. This chance is > > intentional for pre-C++11 compat. > > > > > > Please, read this thread and the other. The warning I originally intended to > fix > should have just been fixed in [1] by Matthew by switching the underlying > value > from signed to unsigned. No, you are missing the point. This is intentionally not using the enum directly as the enum would be larger. Using C++11 sized enums on the other hand created funny ABI changes. Joerg ___ 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
> weird, neither my checkout has it, nor the trunk: Oh my gosh, this is embarrassing: I have ePdfDataType_Unknown = 0xff because I pushed for this change some time ago[1]. I still have it applied in my tree because it because it's the contention used in other PoDoFo enums, and I somehow need it. While you understood the purpose of the change, you ended not applying it since it was causing such warnings, which I fixed in my tree with this change. You wrote: On Sat, 10 Mar 2018 at 16:27, zyx wrote: > > On Thu, 2018-02-22 at 12:20 +0100, Francesco Pretto wrote: > > -ePdfDataType_Unknown/**< The Datatype is unknown > > */ > > +ePdfDataType_Unknown = 0xff /**< The Datatype is unknown */ > > Hi, > this change generates tons of compiler warnings like this one: >src/base/PdfVariant.h: In member function ‘bool > PoDoFo::PdfVariant::IsDirty() const’: >src/base/PdfVariant.h:954:9: warning: case label value exceeds maximum > value for type > thus I'm against it, even in cleans the code a bit. I ask you if we can move on with the change applied by Matthew, which I think is wise and has the benefit of accommodating the change I was aiming for. If you are reverting I will not complain but I kindly ask you to comment the code with a warning so nobody will attempt to change that underlying type anymore: I was confused enough and surely the code could have been more talkative in this case. [1] https://sourceforge.net/p/podofo/mailman/podofo-users/thread/0439eeb1a488653f84f7d66ea69f027cec4e6b97.camel%40gmx.us/#msg36249129 On Wed, 23 Jan 2019 at 12:39, zyx wrote: > > Hi, > > On Wed, 2019-01-23 at 10:35 +0100, Francesco Pretto wrote: > > it's about ePdfDataType_Unknown = 0xff, which is > > definitely 255: > > weird, neither my checkout has it, nor the trunk: > https://sourceforge.net/p/podofo/code/HEAD/tree/podofo/trunk/src/base/PdfDefines.h#l211 > nor 0.9.6 branch: > https://sourceforge.net/p/podofo/code/HEAD/tree/podofo/branches/PODOFO_0_9_6_BRANCH/src/base/PdfDefines.h#l211 > > Do I miss anything there? > Bye, > zyx > > > > > ___ > 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 Mon, Jan 14, 2019 at 11:08:03PM +0100, Francesco Pretto wrote: > > From: Francesco Pretto > > Fixes warnings in linux since the maximum value of EPdfDataType is 255 Please don't do this and look at the history of the code. This chance is intentional for pre-C++11 compat. Joerg ___ 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 Wed, 2019-01-23 at 10:35 +0100, Francesco Pretto wrote: > it's about ePdfDataType_Unknown = 0xff, which is > definitely 255: weird, neither my checkout has it, nor the trunk: https://sourceforge.net/p/podofo/code/HEAD/tree/podofo/trunk/src/base/PdfDefines.h#l211 nor 0.9.6 branch: https://sourceforge.net/p/podofo/code/HEAD/tree/podofo/branches/PODOFO_0_9_6_BRANCH/src/base/PdfDefines.h#l211 Do I miss anything there? 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
On Wed, 23 Jan 2019 at 12:18, Joerg Sonnenberger wrote: > > On Mon, Jan 14, 2019 at 11:08:03PM +0100, Francesco Pretto wrote: > > > > From: Francesco Pretto > > > > Fixes warnings in linux since the maximum value of EPdfDataType is 255 > > Please don't do this and look at the history of the code. This chance is > intentional for pre-C++11 compat. > > Please, read this thread and the other. The warning I originally intended to fix should have just been fixed in [1] by Matthew by switching the underlying value from signed to unsigned. [1] https://sourceforge.net/p/podofo/code/1959/ . ___ 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, 23 Jan 2019 at 10:12, zyx wrote: > P.S.: it would be nice to keep conversation on specific patch proposal, > not on random mail, where only its subject is changed, but the > threading is all wrong > I definitely agree. > > On Mon, 2019-01-14 at 23:08 +0100, Francesco Pretto wrote: > > Fixes warnings in linux since the maximum value of EPdfDataType is > > 255 > > Hi, > I do not see that warning, and the maximum value of EPdfDataType is 10, > not 255 (11 values in the enum, 0 .. 10 inclusive). Maybe it's shown in > combination with other compiler options? I think I had the warning with clang (I can't confirm now, I'm compiling podofo with gcc/clang/VisualStudio). I quote myself in the other thread, it's about ePdfDataType_Unknown = 0xff, which is definitely 255: On Wed, 23 Jan 2019 at 09:05, Francesco Pretto wrote: > Forgot to say: it passed some time so I don't remember precisely but > the change I did was mostly to fix warning issued by gcc or clang on > the switch of m_eDataType in PdfVariant.h inline methods. The maximum > value of EPdfDataType is ePdfDataType_Unknown = 0xff, so the switch > was complaining that one was switching on signed integer with values > that overflows the max integer value. So yes: switching to pdf_uint8 > should fix the warning I intended to fix. > ...so I'm currently favorable with the signed->unsigned change if it fixes the possible warning. ___ 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 Mon, 2019-01-14 at 23:08 +0100, Francesco Pretto wrote: > Fixes warnings in linux since the maximum value of EPdfDataType is > 255 Hi, I do not see that warning, and the maximum value of EPdfDataType is 10, not 255 (11 values in the enum, 0 .. 10 inclusive). Maybe it's shown in combination with other compiler options? I do not like mabri's change [2] to pdf_uint8 personally. Enums can have negative numbers, like the ePdfFilter_None is, thus if anyone ever considers changing ePdfDataType_Unknown the same way, which would make sense, then the switch to unsigned integer breaks the code. On Tue, 2019-01-22 at 23:16 +0100, Francesco Pretto wrote: > I think having enum as instance member is a > non issue when using gcc and clang unless one is using " > -fshort-enums" [1]. Only switching to C++11 type safe enums can > enforce size of enums in the standard. Personally, I don't like the > choice of using fixed size integers instead of enums when the problem > of predicable ABI is completely ignored everywhere else in the > library. I'm not sure if you followed the whole history behind the change. As mabri said, the main aim is a reduction of memory usage for an object which is instantiated a lot of times (the other objects with enums are not created that many times as the PdfVariant). The initial change was to make the enum smaller by the compiler when certain C++ standard had been used. That led to issues when the library had been compiled with it on, but the library user had it off (used older C++ standard for compilation), or vice versa. Usage of -fshort-enums has the same effect, both the library builder and the library user should use it or not, not only one of them. If I understand it correctly. Bye, zyx P.S.: it would be nice to keep conversation on specific patch proposal, not on random mail, where only its subject is changed, but the threading is all wrong [2] http://sourceforge.net/p/podofo/code/1959 ___ Podofo-users mailing list Podofo-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/podofo-users
