[cfe-dev] Diagnosing initialization of deprecated data member?
Stephan Bergmann via cfe-dev
cfe-dev at lists.llvm.org
Wed May 13 00:25:01 PDT 2020
On 12/05/2020 20:30, Richard Smith wrote:
> On Tue, 12 May 2020 at 00:48, Stephan Bergmann via cfe-dev
> <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>
> On 11/05/2020 06:55, Richard Smith wrote:
> > On Thu, 7 May 2020 at 01:52, Stephan Bergmann via cfe-dev
> > <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
> <mailto:cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>>> wrote:
> >
> > For
> >
> > > $ cat test.cc
> > > struct S {
> > > int x;
> > > [[deprecated]] int y;
> > > };
> > > int main() {
> > > S s{0, 0};
> > > }
> >
> > Clang emits a diagnostic
> >
> > > $ clang++ test.cc
> > > test.cc:6:12: warning: 'y' is deprecated
> [-Wdeprecated-declarations]
> > > S s{0, 0};
> > > ^
> > > test.cc:3:7: note: 'y' has been explicitly marked
> deprecated here
> > > [[deprecated]] int y;
> > > ^
> > > 1 warning generated.
> >
> > while e.g. GCC and MSVC do not (as checked with godbolt.org
> <http://godbolt.org>
> > <http://godbolt.org>).
> >
> > Is this deliberate behavior on Clang's part, or just a
> consequence of
> > how the code happens to work? Are there opinions on whether
> or not the
> > behavior is useful?
> >
> >
> > I think it's probably a bug. The C++11 attribute is documented as
> > deprecating the name, not the existence of the entity, so
> diagnosing a
> > use that doesn't use the name seems somewhat questionable. (It's not
> > obviously a bad choice, though, since the intent is very likely
> to catch
> > cases that would break if the field is removed, not only if it's
> renamed.)
> >
> > There are a few other warnings channeled through the same path
> > (`DiagnoseUseOfDecl`), such as "availability" warnings, and it's
> quite
> > likely that some of those *should* still be issued for such cases.
> >
> > (I came across this issue when seeing a commit in LibreOffice
> where some
> >
> > > #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> >
> > were added apparently to silence exactly the above situation
> in code
> > initializing a PyTypeObject struct from
> > /usr/include/python3.8/cpython/object.h, not even realizing
> that that
> > pragma would only be needed by Clang and not by GCC.
> >
> > FWIW, the standard's recommended practice is "to produce a
> diagnostic
> > message in case the program refers to a name or entity other
> than to
> > declare it", [dcl.attr.deprecated]/4.)
> >
> >
> > What was the intent in the LibreOffice case? Would they want a
> warning
> > on "S s{.x = 0, .y = 0};" but not on "S s{0, 0};"?
>
> That Python/LibreOffice case is a bit complex, the commit message of
> <https://git.libreoffice.org/core/+/23d9966751566028c50ca95ca203d20f36c64f30%5E%21>
>
> "Fix initialization of Python-3.8--only at-end tp_print member" has the
> details. The intend on the Python side, where the deprecation
> attribute
> got added, is apparently to warn about "initialization by assignment"
> cases like
>
> X.tp_print = 0;
>
> It presumably did not intend to cause warnings in list-initialization
> scenarios as used in the LibreOffice code---I guess the re-addition of
> the dummy tp_print member at the end didn't even take into account the
> -Wmissing-field-initializers for client code using list-initialization,
> which causes client code to silence that warning by adding an
> initializer for the dummy member, but which in turn causes
> -Wdeprecated-declarations.
>
>
> The interaction between this flag and -Wmissing-field-initializers seems
> unfortunate. I wonder whether it would be reasonable to suppress that
> warning for the case where the field is deprecated.
>
> The deprecated field is followed by a #ifdef'd extra field:
>
> > /* these must be last and never explicitly initialized */
> > Py_ssize_t tp_allocs;
>
> I think that might be OK: assuming you don't provide an initializer for
> that field, you'd still get a -Wmissing-field-initializers warning in
> the case where the field exists, but this struct is just fundamentally
> incompatible with -Wmissing-field-initializers in the case where that
> field exists.
>
> For designated initializer scenarios, the warning is apparently also
> useful (and likely intended by the Python authors) in C code, where it
> would warn about legacy code like
>
> ..., .tp_dealloc=..., .tp_print=..., .tp_getattr=..., ...
>
> that assumes tp_print at its original position in PyTypeObject. (For
> C++20, such code would cause an error anyway because tp_print has been
> moved relative to the other members.)
>
>
> I suspect the Python authors do not intend for the tp_print field to be
> explicitly initialized, whether named or not. And given the comment on
> the tp_allocs field, I suspect they do not intend to support use of that
> header with -Wmissing-field-initializers.
>
> So, in short, for the particular scenario at hand, it looks like not
> emitting -Wdeprecated-declarations in the non-designated
> initializer-list scenario would be the most useful behavior.
>
>
> I don't find this particular case to be strongly persuasive one way or
> the other. But I'm slightly leaning towards it being desirable to warn
> in this case: providing an explicit initializer (with a /*tp_print*/
> comment, no less) seems fundamentally no different from providing a
> designated .tp_print= initializer, which seems fundamentally no
> different from providing an x.tp_print= assignment.
Indeed, not emitting -Wmissing-field-initializers for deprecated members
at the end of a struct would look like the best solution in this
particular case. But would it be a good choice in general, and is
"deprecated members a the end of a struct" a common enough scenario to
even bother? After all, maybe it is best to just leave things as they
are now...
More information about the cfe-dev
mailing list