[libcxx-commits] [libcxx] [libcxx] Replace a few _LIBCPP_INLINE_VISIBILITY with _LIBCPP_HIDE_FR… (PR #66661)
via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Sep 18 09:15:13 PDT 2023
philnik777 wrote:
> The cost is that someone has to scroll through this for 10 minutes. That's faster than most actual reviews.
>
> The comment was added in [cb3eb30](https://github.com/llvm/llvm-project/commit/cb3eb30636c8d9136c8a01126963eded45617531), so if things change at the rate they've been changing at, they won't.
>
> I'm also happy to split this in 10 (or whatever) PRs if you think that helps. But just doing it seems reasonably quick, so I'm not sure spending a lot of time discussing the process is worth the time the discussion takes.
The cost isn't that it takes long to review. The problem is that this affects lots of in-flight PRs, resulting in a lot of merge conflict, and probably also a lot of downstream churn. If it were the 10 minutes it takes to look at these changes, I'd be more than happy to take it. FWIW the rate has been a lot higher recently, since we started to actively update the style (not just `_LIBCPP_INLINE_VISIBILITY`) before or after we touch some region of code, since the churn is already there in that case anyways. I'm also happy to take these changes in parts of the code where nobody working on a patch, but that's a lot more effort than just changing all the uses.
https://github.com/llvm/llvm-project/pull/66661
More information about the libcxx-commits
mailing list