MyDeveloperDay added a comment. In D55433#1325117 <https://reviews.llvm.org/D55433#1325117>, @lebedev.ri wrote:
> In D55433#1323779 <https://reviews.llvm.org/D55433#1323779>, @lebedev.ri > wrote: > > > In D55433#1323757 <https://reviews.llvm.org/D55433#1323757>, > > @MyDeveloperDay wrote: > > > > > a lot of projects aren't setup for c++17 yet which is needed for > > > [[nodiscard]] to be allowed, > > > > > > You can use `[[clang::warn_unused_result]]` for this evaluation, that does > > not require C++17. > > > > > But the clang-tidy -fix doesn't break the build, the patch is large but > > > nothing broke. (at compile time at least!) > > > > > > {F7661182} > > > > Uhm, so there wasn't a single true-positive in protobuf? > > > To elaborate, what i'm asking is: > > - Is that project -Werror clean without those changes? > - if yes, after all those changes, does the -Werror build break? > - If it does break, how many of those issues are actual bugs (ignored > return when it should not be ignored), and how many are noise. > - If not, then i guess all these were "false-positives" I totally get where you are coming from, and I want to answer correctly... 1. protobuf was clean to begin with when compiling with -Werror 2. after applying nodiscard fix-it protobuf would not build cleanly with -Werror but will build cleanly without it 3. those warnigns ARE related to the introduction of the [[nodiscard]] F7673134: image.png <https://reviews.llvm.org/F7673134> taking the first one as an example, I'm trying to determine if int ExtensionSet::NumExtensions() const { int result = 0; ForEach([&result](int /* number */, const Extension& ext) { ^^^^^^^ if (!ext.is_cleared) { ++result; } }); return result; } having the checker added nodiscard.. template <typename KeyValueFunctor> [[nodiscard]] KeyValueFunctor ForEach(KeyValueFunctor func) const { if (PROTOBUF_PREDICT_FALSE(is_large())) { return ForEach(map_.large->begin(), map_.large->end(), std::move(func)); } return ForEach(flat_begin(), flat_end(), std::move(func)); } Is a false positive, or is Visual C++ somehow thinking the return value is not used when its a lambda? but I get similar issues when compiling with the Intel compiler, so I think its more likely that the return value from ForEach isn't actually used, and the implementation is just allowing ForEach()'s to be chained together F7673175: image.png <https://reviews.llvm.org/F7673175> All of these new warnings introduced are related to the use of a lambda with the exception of // We don't care what this returns since we'll find out below anyway. pool_->TryFindFileInFallbackDatabase(proto.dependency(i)); Which you could say that it should be written as [[maybe_unused]] auto rt= pool_->TryFindFileInFallbackDatabase(proto.dependency(i)); But the effect of applying the fix-it, doesn't itself break the build, its the effect of [[nodiscard]] having been added. (perhaps incorrectly) However the build is not broken when NOT using warnings as errors, and so it depends on the "ethos" of clang-tidy, if the goal is for clang-tidy to remain error/warning free after applying -fix then i can see the fix-it generating changes that causes warnings isn't correct and that should be considered a false positive and I should consider not emitting the [[nodiscard]] when the argument is perhaps a lambda. I guess in the past I've used clang-tidy to also help me generate new warnings so I can fix that code, but that may not be the goal of its usage. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55433/new/ https://reviews.llvm.org/D55433 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits