[PATCH] D113148: Add new clang-tidy check for string_view(nullptr)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D113148#3109190 <https://reviews.llvm.org/D113148#3109190>, @CJ-Johnson wrote:

> As for "we should be careful we're not too onerous when users enable all `bugprone` checks together.", these fixes are about preventing UB. While I did put this in the "bugprone" module, perhaps "prone" is the wrong way to think about it. It's unconditionally UB in all cases, not just a potential bug. The suggested fixes are important for preventing crashes in the short term, but more importantly for allowing the code to build in the future, since the constructor overload `basic_string_view(nullptr_t) = delete;` is being added.

The concern I have is that a user passes `-checks=-*, bugprone-*` and gets two diagnostics on the same line for the same issue. There is precedence, but usually clang-tidy only gets in this situation when the user enables checks from different modules (I don't know of any case where it happens within the same module). Your existing test cases have significant coverage already: https://godbolt.org/z/49nGYEGzq so this isn't a theoretical concern -- anyone checking against all bugprone (which is pretty typical) will get duplicate diagnostics. So we're in a bit of a novel situation with this patch, and I'm not certain what the correct answer is. I see a few paths:

- accept that there are two diagnostics for the same code construct
- remove the string_view nullptr checking functionality from bugprone-string-constructor so it only lives in the new check
- remove the constructor checking functionality from the new check so it only lives in bugprone-string-constructor
- have both of the checks look to see whether the other check is enabled before issuing the diagnostic and pick one of them to be the "winner" so there's only one diagnostic

There's pros and cons to all of these approaches (and maybe there are other approaches to consider as well). I sort of lean towards the last option, but that could be confusing for people trying to suppress diagnostics. e.g., they could write `NOLINT(bugprone-string-constructor)` to silence the diagnostic and then they get a different diagnostic they also have to silence. However, I also sort of think that confusion is not likely to happen in practice because 1) users are more likely to fix their code than silence *this* warning, 2) users are more likely to write `NOLINT` without a specific check, so all tidy diagnostics would be suppressed. But it's still a bit of concern as a precedent.

@alexfh -- as code owner, do you have thoughts here?



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp:84
+
+    (void)(std::string_view({nullptr})) /* a3 */;
+    // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: constructing basic_string_view from null is undefined; replace with the default constructor
----------------
This (and many others) also generates `-Wbraced-scalar-init`, is that intentional?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113148