This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Warn on including headers that are deprecated in C++17
AbandonedPublic

Authored by Mordante on Mar 22 2023, 5:13 PM.

Details

Reviewers
cjdb
philnik
ayzhao
Group Reviewers
Restricted Project
Summary

The headers ccomplex, cstdbool, and ctgmath have been marked as
deprecated in C++17, so this patch adds a #warning preprocessor
directive to these headers if they're used in a context where the C++
version is >= 17.

Additionally, update tests and remove includes of deprecated headers in
existing non-deprecated headers.

Diff Detail

Event Timeline

ayzhao created this revision.Mar 22 2023, 5:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 5:13 PM
ayzhao requested review of this revision.Mar 22 2023, 5:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 5:13 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ayzhao retitled this revision from [libc++] Warn on including headers deprecated in C++17 to [libc++] Warn on including headers that are deprecated in C++17.Mar 22 2023, 5:16 PM
cjdb added inline comments.Mar 22 2023, 5:24 PM
libcxx/include/ccomplex
24
  • "removed as of C++20" increases the urgency of the diagnostic.
  • "Header" bit removed since that will be implied by context.
  • Added backticks to support SARIF diagnostics rendering this in Markdown.

Same comment applies below.

ayzhao updated this revision to Diff 507561.Mar 22 2023, 5:32 PM
ayzhao marked an inline comment as done.

update warning message per review comments

ayzhao updated this revision to Diff 507563.Mar 22 2023, 5:41 PM

remove quotes from warning message

cjdb accepted this revision.Mar 22 2023, 5:43 PM

LGTM from a diagnostic perspective!

thakis added a subscriber: thakis.Mar 23 2023, 12:14 AM

Ms stl uses this construction to make the warning grouped under -Wdeprecated-declarations: https://github.com/search?q=repo%3Amicrosoft%2FSTL%20_CXX17_DEPRECATE_C_HEADER&type=code

Many worth doing?

philnik requested changes to this revision.Mar 23 2023, 2:51 AM
philnik added a subscriber: philnik.
philnik added inline comments.
libcxx/include/ccomplex
24

I don't think we want to say "and removed in C++20" if we don't actually remove the header. We'd have to move these headers into their own directory and teach clang about that, and this seems like quite a bit of work to remove these headers (although it might clean up the code base a bit).

24

Otherwise it just results in problems. Clang should just remove the quotes if the warning consists only of a string literal.

libcxx/include/complex.h
27

Why do we need this include now?

libcxx/test/libcxx/language.support/support.runtime/version_cstdbool.pass.cpp
11

Where did you get c++0x and c++1y from? I can't find anything in the code base.

libcxx/test/libcxx/transitive_includes.sh.cpp
52
libcxx/utils/generate_header_tests.py
64–67

We shouldn't disable the tests just because the headers are deprecated. It's still fine to use them in C++17.

This revision now requires changes to proceed.Mar 23 2023, 2:51 AM
ayzhao updated this revision to Diff 507797.Mar 23 2023, 10:28 AM
ayzhao marked 5 inline comments as done.

code review comments

Ms stl uses this construction to make the warning grouped under -Wdeprecated-declarations: https://github.com/search?q=repo%3Amicrosoft%2FSTL%20_CXX17_DEPRECATE_C_HEADER&type=code

Many worth doing?

The problem with this approach (and with #pragma message, as I discovered earlier), is that the warning doesn't show up if the path to libc++ is passed via -isystem:

$ cat /tmp/llvm/include/c++/v1/cstdbool
// -*- C++ -*-
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef _LIBCPP_CSTDBOOL
#define _LIBCPP_CSTDBOOL

/*
    cstdbool synopsis

Macros:

    __bool_true_false_are_defined

*/

#include <__assert> // all public C++ headers provide the assertion handler
#include <__config>

using _HEADER_cstdbool [[deprecated("hello, world")]] = int;
using _Hdr_cstdbool = _HEADER_cstdbool ;

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#  pragma GCC system_header
#endif

#undef __bool_true_false_are_defined
#define __bool_true_false_are_defined 1

#endif // _LIBCPP_CSTDBOOL

$ cat include-deprecated.cc
#include <cstdbool>

$ clang++ -nostdinc++ -isystem/tmp/llvm/include/c++/v1 -c -o /dev/null include-deprecated.cc -std=c++20 -Wdeprecated-declarations
(nothing shows up)

@cjdb suggested implementing this with #warning, but I'll leave it up to the libc++ maintainers to decide on the best way forward. Any thoughts @philnik ?

libcxx/include/complex.h
27

Removed. I included it here since <ccomplex> originally included it, but now that I look at it, it seems like it doesn't matter.

libcxx/test/libcxx/language.support/support.runtime/version_cstdbool.pass.cpp
11
cjdb added inline comments.Mar 23 2023, 12:43 PM
libcxx/include/ccomplex
24

I don't think we want to say "and removed in C++20" if we don't actually remove the header. We'd have to move these headers into their own directory and teach clang about that, and this seems like quite a bit of work to remove these headers (although it might clean up the code base a bit).

It's implementable in library.

#if __cplusplus >= 202003L
#  error `<ccomplex>` removed in C++20.
#elif __cplusplus == 201703L
#  warning `<ccomplex>` deprecated in C++17
#endif

Otherwise it just results in problems. Clang should just remove the quotes if the warning consists only of a string literal.

What problems are those?

I have not looked closely at the patch, can you please add the paper number in the summary and update the status page
libcxx/doc/Status? That makes reviewing the changes a lot easier.

libcxx/test/std/language.support/support.runtime/cstdbool.pass.cpp
11

Why not using the UNSUPPORTED like we usually do?

I have not looked closely at the patch, can you please add the paper number in the summary and update the status page
libcxx/doc/Status? That makes reviewing the changes a lot easier.

The headers were marked deprecated in p0063r3, which, interestingly enough, is already marked complete in the status page.

libcxx/test/std/language.support/support.runtime/cstdbool.pass.cpp
11

My idea was that by using XFAIL, we can test that the deprecation warning works (since we build with -Werror).

philnik added inline comments.Mar 27 2023, 2:00 AM
libcxx/include/ccomplex
24

That would break __has_include(<ccomplex>) though. What's the point of that feature if you can't actually rely on it?

The compilers sometimes produce random additional warnings: https://godbolt.org/z/fE7x6E11r

libcxx/include/tgmath.h
27–29

Also here.

libcxx/test/libcxx/assertions/headers_declare_verbose_abort.sh.cpp
49

Also for the comment in the other tests.

50

and move it near the XFAIL above.

libcxx/test/libcxx/language.support/support.runtime/version_cstdbool.pass.cpp
11

You've got it the wrong way around :). The code you linked to treats them as equal compiler flags, i.e. -std=c++11 is the same as -std=c++0x and c++11 is the lit feature that's set. So you only have to test c++11 || c++14 || c++17 (and disable the deprecation warning).

libcxx/test/std/language.support/support.runtime/cstdbool.pass.cpp
11

That should instead be done with a .verify.cpp. You can also run the test suite without warnings enabled, and having an XFAIL from a warning would break this. The .verify.cpp also checks what warnings/errors are generated instead of just XFAILing for any reason.

Mordante requested changes to this revision.Apr 8 2023, 5:09 AM

I have not looked closely at the patch, can you please add the paper number in the summary and update the status page
libcxx/doc/Status? That makes reviewing the changes a lot easier.

The headers were marked deprecated in p0063r3, which, interestingly enough, is already marked complete in the status page.

Thanks for the info.

libcxx/test/libcxx/assertions/headers_declare_verbose_abort.sh.cpp
50

This will fail when running the tests with _LIBCPP_DISABLE_DEPRECATION_WARNINGS already defined.

I'm not really happy with blanket disabling the deprecation warnings in tests. Basically this changes the behaviour of these tests and they are not tested in the typical libc++ configuration. I really would like to see a more fine-grained approach in the basic generation script instead of this approach.

This revision now requires changes to proceed.Apr 8 2023, 5:09 AM
Mordante commandeered this revision.Aug 31 2023, 12:27 PM
Mordante edited reviewers, added: ayzhao; removed: Mordante.

This patch is inactive for quite a while. We're moving to GitHub and want to clean up the patch review queue. If you want to pursue this patch, please file a new patch as GitHub PR. I'll close the patch for now,

Mordante abandoned this revision.Aug 31 2023, 12:27 PM

This patch is inactive for quite a while. We're moving to GitHub and want to clean up the patch review queue. If you want to pursue this patch, please file a new patch as GitHub PR. I'll close the patch for now,

Sorry for leaving this patch in your review queue for such a long time - I haven't had the bandwidth to move this forward, and I doubt I will in the foreseeable future.