[libcxx-commits] [PATCH] D145508: [libc++] Reject standard attributes which are extensions in libcpp-uglify-attributes

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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



================
Comment at: libcxx/include/barrier:133
     }
-    [[nodiscard]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
+    [[__nodiscard__]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     arrival_token arrive(ptrdiff_t __update)
----------------
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?


================
Comment at: libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp:50
+std::span<const char* const> get_standard_attributes(const clang::LangOptions& lang_opts) {
+  size_t attribute_count = 2;
+
----------------
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.


================
Comment at: libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp:86
 std::optional<std::string> getUglyfiedCXX11Attr(const clang::Attr& attr) {
-  // Don't try to fix attributes with `using` in them.
-  if (std::ranges::search(std::string_view(attr.getSpelling()), std::string_view("::")).empty())
-    return std::nullopt;
+  // TODO: Don't try to fix attributes with `using` in them.
 
----------------
philnik wrote:
> The old way is fundamentally broken. IMO it's better to produce slightly wrong hints in an edge case instead of never producing hints.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145508/new/

https://reviews.llvm.org/D145508