Hello Hib,

I am not sure whether it is good style to work the solution of this implementation problem into a public header. At least for GCC 4.6 and later, it should be possible to resolve this locally using e.g.

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
Annotation::Annotation( AnnotationPrivate &dd, const QDomNode &annNode )
    : d_ptr( &dd )
{
[...]
}
#pragma GCC diagnostic pop

Not sure what is worse, putting it into the headers or using a solution that only works for a single compiler. :-(

Created attachment 71193 [details] [review]
deprecated-warning.patch

(In reply to comment #1)

> I am not sure whether it is good style to work the solution of this
> implementation problem into a public header.

You are right. I reworked my patch to a more generic solution defining POPPLER_DECL_DEPRECATED to replace Q_DECL_DEPRECATED.

What do you think of it?

I don't think the current patch is good, it'd let me use window anywhere and i wouldn't realize it's wrong, it would be acceptable if you wrappred somehow the "deprecated but needed" code in  the constructors, no idea how to achieve that though

(In reply to comment #3)
> I don't think the current patch is good, it'd let me use window anywhere and
> i wouldn't realize it's wrong

Do you really mean "anywhere", or just "anywhere in poppler-annotation.cc"?
If it is the first, my patch is certainly not good.

However, I think it can not be avoided that you can use window anywhere in poppler-annotation.cc. It is upto the poppler developpers to know that it is deprecated and only part of the implementation for backward compatibility.

I meant "anywhere in poppler-annotation.cc"

Sure, i agree it's up to us to know that stuff, but having a new warning creep up is an easy way to get the signal that something might not be correct. 

All in all i think that the added "security" that warning gives me is better than silencing some warnings that i already know are going to be there.

Anyone else has an opinion on this?

Created attachment 71211 [details] [review]
Locally disable deprecation warnings

Hello,

Personally, I think those warnings are rather helpful since they remind us that this deprecated member should go away at some point in time. (Even though I don't know how deprecation periods are currently handled in Poppler.)

Besides that, I think you really want to temporarily and locally disable the warning and not the deprecation itself. If one really wants to do this and still tackle several compilers, maybe something like the attached patch could be used.

But still, I think the warning should stay with a long-term plan on removing the offender, i.e. the declaration itself.

Regards, Adam.

Created attachment 71212 [details] [review]
Locally disable deprecation warnings

Sorry, there was superfluous BLOCK/RESET pair in the first patch...

I like Adam's patch better, but he agrees with me that the warnings are "good", let's see if someone else comments before we apply/reject the patch

Created attachment 71217 [details] [review]
Locally disable deprecation warnings

Better add a trivial fallback for unknown compiler if this is going into master.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/218.