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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

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

> In D113148#3108993 <https://reviews.llvm.org/D113148#3108993>, @aaron.ballman wrote:
>
>> Generally speaking, we prefer to improve the existing checks. I think `bugprone-string-constructor` would probably be a better place for the constructor-related functionality. But that still means we don't have a good place for things like the assignment and comparison functionality, and it seems more useful to keep all of the `string_view`-with-`nullptr` logic in one place. That said, we should be careful we're not too onerous when users enable all `bugprone` checks together.
>
> 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.

@aaron.ballman Maybe we should do something to ease the burden of `bugprone-` becoming a catch-all basket, and create a new checker group for the "unconditional UB" or "in almost all cases this will make you do bad things" kinds of stuff.



================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:82-90
+  Checks for various ways that the ``nullptr`` literal can be passed into the
+  ``const CharT*`` constructor of ``std::basic_string_view`` and replaces them
+  with the default constructor in most cases. For the comparison operators,
+  braced initializer list does not compile so instead the empty string is used.
+  Also, ``==`` and ``!=`` are replaced with calls to ``.empty()``.
+
+  This prevents code from invoking behavior which is unconditionally undefined.
----------------
Usually we only put a one sentence short summary into the release notes.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:13-14
+This prevents code from invoking behavior which is unconditionally undefined.
+The single-argument ``const char*`` constructor of ``std::string_view`` does
+not check for the null case before dereferencing its input.
+
----------------
You mentioned in the review discussion that a constructor taking `std::nullptr_t` will be added with an explicit `= delete;`. Maybe it is worth mentioning it here, if we have a standard version or a proposal on track for changing the STL like that?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:16-17
+
+.. code-block:: c++
+  std::string_view sv = nullptr;
+
----------------
Will this work? Isn't the `:` making RST confused here? You usually need an empty line because the block "instance" may be given additional options in RST.

It might compile the docs still, so this is the kind of thing that must be verified visually.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:46
+
+Note: The source pattern with trailing comment "A" selects the
+``(const CharT*)`` constructor overload and then value-initializes the pointer,
----------------
There should be a `.. note::` block which renders a nice box of note with an icon.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:49
+causing a null dereference. It happens to not include the ``nullptr`` literal,
+but it is still within the scope of this ClangTidy check.
+
----------------
Clang-Tidy. Or just simply "this check".


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:53
+``(const CharT*, size_type)`` constructor which is perfectly valid, since the
+length argument is ``0``. It is not changed by this ClangTidy check.
----------------
ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113148