[libcxx-commits] [PATCH] D114915: [libc++][NFC] Address clang-tidy warnings in include/

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a subscriber: cjdb.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__algorithm/copy.h:72
+                        _VSTD::__unwrap_iter(__result)));
 }
 
----------------
FWIW, I find the left-hand code easier to read and easier to safely-modify than the right-hand code. (This applies, for me, everywhere clang-tidy is introducing early-returns and/or removing braces from `if`-bodies.)
However, if we really want to do this, I won't loudly object.


================
Comment at: libcxx/include/__functional/bind.h:73
 
-}  // placeholders
+} // namespace placeholders
 
----------------
These comment-only diffs are clearly a good idea and you should just land them immediately in their own commit, in order to shorten this PR. (Maybe wait for @ldionne or @mordante or @cjdb to +1 this comment, for the sake of protocol.)


================
Comment at: libcxx/include/__functional/hash.h:225-230
+  if (__len <= 16)
+    return __hash_len_0_to_16(__s, __len);
+  if (__len <= 32)
+    return __hash_len_17_to_32(__s, __len);
+  if (__len <= 64)
     return __hash_len_33_to_64(__s, __len);
----------------
The original binary-tree-of-`if`s was clearly done intentionally, and this is tight-loop code; but if I had to bet one way or the other, I guess I'd bet this refactor was fine. If done, it should be landed in its own commit, for ease of bisecting.


================
Comment at: libcxx/include/__memory/auto_ptr.h:41-42
+
+    // Reason: non-const auto_ptr& is required by the C++ standard
+    // NOLINTNEXTLINE(misc-unconventional-assign-operator)
     _LIBCPP_INLINE_VISIBILITY auto_ptr& operator=(auto_ptr& __p) _NOEXCEPT
----------------
This specific suppression seems OK, but then you do this like a million more times, for other assignment operators, for various reasons such as "oh no it uses `enable_if`" :)
I think you should just add `-misc-unconventional-assign-operator` to the `.clang-tidy` file. That instantly removes like half of the diffs in this PR.


================
Comment at: libcxx/include/regex:2664-2672
     _LIBCPP_INLINE_VISIBILITY
-    basic_regex& operator=(const value_type* __p)
-        {return assign(__p);}
+    basic_regex& operator=(const value_type* __p) { assign(__p); return *this; }
 #ifndef _LIBCPP_CXX03_LANG
     _LIBCPP_INLINE_VISIBILITY
-    basic_regex& operator=(initializer_list<value_type> __il)
-        {return assign(__il);}
+    basic_regex& operator=(initializer_list<value_type> __il) { assign(__il); return *this; }
 #endif // _LIBCPP_CXX03_LANG
     template <class _ST, class _SA>
----------------
What was clang-tidy complaining about here, specifically?
Don't these diffs change a tail-call into a non-tail-call, and isn't that generally a bad thing? (Of course in these cases the optimizer will DTRT anyway, but I bet it's a pessimization for `-O0` mode.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114915