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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM! I added some comments, but only feedback and no changes are needed.



================
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)
----------------
ldionne wrote:
> philnik wrote:
> > Mordante wrote:
> > > 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?
> > `<barrier>` itself is an extension in C++14 and C++17. This is how I noticed that we have them as extensions.
> 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.
Thanks for the comments. I still don't like it, but no objection either.


================
Comment at: libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp:29
+std::vector<const char*> get_standard_attributes(const clang::LangOptions& lang_opts) {
+  std::vector<const char*> attributes = {"noreturn", "carries_dependency"};
+
----------------
Interesting I recalled more attributes in C++11, but according to cppreference this is correct.


================
Comment at: libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp:49
+
+std::span<const char* const> get_standard_attributes(const clang::LangOptions& lang_opts) {
+  size_t attribute_count = 2;
----------------
ldionne wrote:
> 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.
+1 I really like this a lot better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145508