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

[philnik]

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

[philnik]

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.

[Mordante]

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]

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]

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]

Address comments

[philnik]

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.