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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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.
 
----------------
ldionne wrote:
> 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.
I've reworded the TODO for now. I'd really like to fix the problem, so I'd rather keep the TODO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145508