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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In general, I like the comment fixes and some of the reformatting, however I don't see the point of most of the control flow changes, and in many cases it felt to me like the status quo was better.



================
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
               {
----------------
Honestly, I find the left hand side easier to read -- I don't understand why clang-tidy is suggesting this change. Anyone?


================
Comment at: libcxx/include/__bit_reference:69
     _LIBCPP_INLINE_VISIBILITY
-    __bit_reference& operator=(const __bit_reference& __x) _NOEXCEPT
-        {return operator=(static_cast<bool>(__x));}
+    __bit_reference& operator=(const __bit_reference& __x) _NOEXCEPT {
+      operator=(static_cast<bool>(__x));
----------------
This change LGTM.


================
Comment at: libcxx/include/__functional/bind.h:73
 
-}  // placeholders
+} // namespace placeholders
 
----------------
Quuxplusone wrote:
> 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.)
+1


================
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
----------------
Quuxplusone wrote:
> 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.
Agreed -- IMO clang-tidy and such tools are well suited for normal code bases, however this is the Standard Library, and sometimes we have to do things that are outside of what should be considered "normal". For instance, we *have* to leave these unconventional assignments there because they are required by the Standard -- in a normal code base I'd even go further and remove everything that mentions `auto_ptr`, but obviously that doesn't apply to us.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114915