[libcxx-commits] [PATCH] D145508: [libc++] Reject standard attributes which are extensions in libcpp-uglify-attributes
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Apr 7 09:45:45 PDT 2023
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
More information about the libcxx-commits
mailing list