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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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)
----------------
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.


================
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;
----------------
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.


================
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.
 
----------------
Mordante wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145508