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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/minmax_element.h:50
               }
+              if (__comp(*__first, *__i))
+              {
+                  if (__comp(*__first, *__result.first))
+                      __result.first = __first;
+                  if (!__comp(*__i, *__result.second))
+                      __result.second = __i;
+              }
               else
               {
----------------
ldionne wrote:
> Honestly, I find the left hand side easier to read -- I don't understand why clang-tidy is suggesting this change. Anyone?
I think it is 'move the code to the left as much as possible'. I definitely agree with the sentiment, and personally don't really find it harder to read. Do you want to keep `else` in general or only in this specific case?


================
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>
----------------
Quuxplusone wrote:
> 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.)
clang-tidy complains that `operator=()` does not return `*this` (it's another case of `misc-unconventional-assign-operator`). It does of course return `*this` through `assign`, but I think explicitly returning `*this` makes it look less weird. I thought it would compile to the same assembly, but that probably isn't the case in `-O0`. I'll revert the changes when I disable `misc-unconventional-assign-operator`. And what does DTRT stand for?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114915