[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

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