Page MenuHomePhabricator

[libc++] Reject standard attributes which are extensions in libcpp-uglify-attributes
Needs ReviewPublic

Authored by philnik on Tue, Mar 7, 8:02 AM.

Details

Reviewers
ldionne
Mordante
Group Reviewers
Restricted Project
Summary

This adds a list of attributes which can be pretty to be able to reject attributes which were introduced in a later C++ standard.
Fixes #61196

Diff Detail

Unit TestsFailed

TimeTest
270 mslibcxx CI No exceptions > llvm-libc++-shared-cfg-in.std/language_support/support_dynamic/new_delete/new_delete_array::new_align_val_t_nothrow.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fno-exceptions -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/test/std/language.support/support.dynamic/new.delete/new.delete.array/Output/new_align_val_t_nothrow.pass.cpp.dir/t.tmp.exe
270 mslibcxx CI No exceptions > llvm-libc++-shared-cfg-in.std/language_support/support_dynamic/new_delete/new_delete_array::new_array_nothrow.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_array_nothrow.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fno-exceptions -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/test/std/language.support/support.dynamic/new.delete/new.delete.array/Output/new_array_nothrow.pass.cpp.dir/t.tmp.exe
310 mslibcxx CI No exceptions > llvm-libc++-shared-cfg-in.std/language_support/support_dynamic/new_delete/new_delete_single::new_align_val_t_nothrow.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fno-exceptions -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/test/std/language.support/support.dynamic/new.delete/new.delete.single/Output/new_align_val_t_nothrow.pass.cpp.dir/t.tmp.exe
270 mslibcxx CI No exceptions > llvm-libc++-shared-cfg-in.std/language_support/support_dynamic/new_delete/new_delete_single::new_nothrow.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_nothrow.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fno-exceptions -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/f532f67fa57c-1/llvm-project/libcxx-ci/build/generic-noexceptions/test/std/language.support/support.dynamic/new.delete/new.delete.single/Output/new_nothrow.pass.cpp.dir/t.tmp.exe

Event Timeline

philnik created this revision.Tue, Mar 7, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Mar 7, 8:02 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
philnik requested review of this revision.Tue, Mar 7, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Mar 7, 8:02 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik added inline comments.Tue, Mar 7, 8:04 AM
libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp
72

The old way is fundamentally broken. IMO it's better to produce slightly wrong hints in an edge case instead of never producing hints.

Since this fixes a bug, should we add regression tests?

libcxx/include/barrier
133

This looks odd, either we are Standard compliant and we can use [[nodiscard]] or this is an extension in some language modi, but we're not using _LIBCPP_NODISCARD_EXT. Do you know why this is needed here?

libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp
50

This count feels extremely error prone, maybe an array per language version. This then has some duplicates but feels easier to maintain. It also allows for the removal of an attribute in a later version of the standard.

72

The TODO sound do. Don't try to do something seems like maintaining the status-quo, while a todo implies a change.

Can you add in the comment what is "fundamentally" broken?

I don't mind not fixing, but then let's state that and not put a TODO. That way the comment doesn't show when grepping for TODOs.

philnik added inline comments.Tue, Mar 7, 12:29 PM
libcxx/include/barrier
133

<barrier> itself is an extension in C++14 and C++17. This is how I noticed that we have them as extensions.

libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp
50

I don't disagree, but one array per version seems also error-prone.

ldionne added inline comments.Mon, Mar 13, 9:57 AM
libcxx/include/barrier
133

Yeah, this is a weird case but I think using __nodiscard__ is right here. We support barrier in C++14 so we can't use nodiscard, but barrier is specified to have nodiscard unconditionally so using _LIBCPP_NODISCARD_EXT also doesn't make sense.

libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp
49

FWIW I would do this:

std::vector<std::string> get_standard_attributes(const clang::LangOptions& lang_opts) {
  std::vector<std::string> attributes = {
    // C++11 attributes
    "noreturn",
    "carries_dependency"
  };

  if (lang_opts.CPlusPlus14)
    attributes.push_back("deprecated");

  ...

  return attributes;
}

Not quite as efficient, but much more straightforward and TBH efficiency here probably doesn't matter.

72

I am not sure I understand what's your preferred resolution here. Do you want a TODO like @philnik did here, or do you want him to instead explain why this is fundamentally broken (i.e. we don't have the right Clang API) and make it a normal comment, but not a TODO?

Personally, I would go with (2) unless there is a proper solution to fix the TODO, in which case I would do it in this patch.

philnik updated this revision to Diff 506372.Sun, Mar 19, 3:25 AM
philnik marked 3 inline comments as done.

Address comments

philnik added inline comments.Sun, Mar 19, 3:25 AM
libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp
72

I've reworded the TODO for now. I'd really like to fix the problem, so I'd rather keep the TODO.