[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length
Whisperity via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 4 04:42:51 PDT 2018
whisperity added a comment.
Generally grammar / phrasing things that have caught my eye.
The rest of the code looks okay, bar my previous comment about better commenting / separating chunks of helper functions.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:74
+ const MatchFinder::MatchResult &Result) {
+ return Lexer::getLocForEndOfToken(E->getLocEnd(), 0, *Result.SourceManager,
+ Result.Context->getLangOpts());
----------------
`getLocEnd` will be removed, see D50353
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:294
+ Diag << RHSRemoveFix;
+ else if (LengthExpr->getLocEnd() == InnerOpExpr->getLocEnd())
+ Diag << LHSRemoveFix;
----------------
`getLocEnd` and `getLocStart` used here too.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:42
+
+Transformation rules with 'memcpy()'
+------------------------------------
----------------
with -> of
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:46
+It is possible to rewrite the ``memcpy()`` and ``memcpy_s()`` calls as the
+following four function: ``strcpy()``, ``strncpy()``, ``strcpy_s()``,
+``strncpy_s()``, where the latter two is the safe version. Analogly it is
----------------
function**s**
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:47
+following four function: ``strcpy()``, ``strncpy()``, ``strcpy_s()``,
+``strncpy_s()``, where the latter two is the safe version. Analogly it is
+possible to rewrite ``wmemcpy()`` related functions handled in the same way.
----------------
are, versions
(Perhaps as this is user-facing code a half sentence about what safe version means could also be put here.)
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:47
+following four function: ``strcpy()``, ``strncpy()``, ``strcpy_s()``,
+``strncpy_s()``, where the latter two is the safe version. Analogly it is
+possible to rewrite ``wmemcpy()`` related functions handled in the same way.
----------------
whisperity wrote:
> are, versions
>
> (Perhaps as this is user-facing code a half sentence about what safe version means could also be put here.)
Analogly -> Respectively, / In a similar fashion,
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:48
+``strncpy_s()``, where the latter two is the safe version. Analogly it is
+possible to rewrite ``wmemcpy()`` related functions handled in the same way.
+
----------------
``-related
~~handled~~
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:52
+
+- If the type of the destination array is not just ``char``, that means it
+ cannot be any string handler function. But if the given length is
----------------
What is "just char"? Is `unsigned char` or `signed char` not "just char"?
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:52-53
+
+- If the type of the destination array is not just ``char``, that means it
+ cannot be any string handler function. But if the given length is
+ ``strlen(source)`` then fix-it adds ``+ 1`` to it, so the result will be
----------------
whisperity wrote:
> What is "just char"? Is `unsigned char` or `signed char` not "just char"?
"that means it cannot be any string handler function" This sentence doesn't make any sense.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:54
+ cannot be any string handler function. But if the given length is
+ ``strlen(source)`` then fix-it adds ``+ 1`` to it, so the result will be
+ null-terminated.
----------------
Will be, or can be? Simply allocating more memory won't magically make a null terminator happen, I think. (Although I can be wrong.)
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:59
+
+- If the destination cannot overflow then the new function is should be the
+ simple ``cpy()``, because it is more efficient.
----------------
The destination by itself can't overflow as it is a properly allocated memory.
Perhaps you meant "If copy to the destination array cannot overflow"?
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:59
+
+- If the destination cannot overflow then the new function is should be the
+ simple ``cpy()``, because it is more efficient.
----------------
whisperity wrote:
> The destination by itself can't overflow as it is a properly allocated memory.
>
> Perhaps you meant "If copy to the destination array cannot overflow"?
... then the simple `cpy()`-like functions should be used as they are more efficient.
(~~is should be the~~)
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:62
+
+- If the destination can overflow and ``AreSafeFunctionsAvailable = 1`` and it
+ is possible to read the length of the destination array then the new function
----------------
Same, mention that the copy/move/write overflows, not the array itself.
Also, I'd phrase it as such: and the option `AreSafeFunctionsAvailable` is `1`.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:63-64
+- If the destination can overflow and ``AreSafeFunctionsAvailable = 1`` and it
+ is possible to read the length of the destination array then the new function
+ could be safe (``cpy_s()``).
+
----------------
read?
could be the safe version, `cpy_s()`.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:66-67
+
+- If the new function is could be safe and the target environment is C++ then
+ it is not necessary to pass the length of the destination array.
+
----------------
If the new function could be the safe version and C++ files are analysed, the length of the destination array can be omitted.
(the target environment is not C++, but Linux / Apple / PowerPC, etc.)
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:69
+
+and based on the length of the source string:
+
----------------
Start this line with "Rewrite based on" too so the rhythm of the paragraphs are kept.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:71
+
+- If the given length is ``strlen(source)`` (or equals length) then the new
+ function is should be the simple ``cpy()``, because it is more efficient than
----------------
What is an "equals length"?
Perhaps `strlen(source)` or `source.length()`?
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:72-73
+- If the given length is ``strlen(source)`` (or equals length) then the new
+ function is should be the simple ``cpy()``, because it is more efficient than
+ the safe version.
+
----------------
Same comment about the "is should be".
"because it is" can be rephrased as "as it is", and also mention why the safe version shouldn't be used.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:95
+ new function ``strchr``/``wcschr``'s return type is correct.
+ - Also the third argument is not needed.
+
----------------
What is this third argument?
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:100-101
+ ``memmove_s``/``wmemmove_s``, it has four arguments,
+ - the new second argument is the first argument's length, and
+ - the third argument will be moved as the fourth, where ``+ 1`` needed.
+
----------------
Too many indirections here. Instead of specifying the position of the argument, name them: "length of the source string", etc.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:103
+
+ - Else: The third argument gets a ``+ 1`` operation.
+
----------------
The ~~third argument~~ (whatever it actually is as opposed to a positional indirection) is incremented accordingly.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:105
+
+- ``memmove_s function's fourth argument gets a ``+ 1`` operation.
+
----------------
same about "incremented" instead of "getting a +1 operation" (that sounds like a surgery).
Also, the function name's codeblock is not terminated, see the syntax highlight.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:125
+
+ A string which is can be used as an integer. The default value is ``""``.
+ If ``Default``, ``d`` or not set the checker rely on the existence/value of
----------------
> A string which is can be used as an integer.
Okay, let's just bask in the fact that we are C++ people and not mention subtle type conversions from the user into the parser here...
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:125-132
+ A string which is can be used as an integer. The default value is ``""``.
+ If ``Default``, ``d`` or not set the checker rely on the existence/value of
+ the ``__STDC_WANT_LIB_EXT1__`` macro.
+ If ``Yes``, ``y`` or non-zero value the target environment implements ``_s``
+ suffixed memory and character handler functions which are safer than older
+ version (e.g. ``memcpy_s()``).
+ If ``No``, ``n`` or zero value the ``_s`` suffixed functions are not
----------------
whisperity wrote:
> > A string which is can be used as an integer.
>
> Okay, let's just bask in the fact that we are C++ people and not mention subtle type conversions from the user into the parser here...
Generally if you can (RST syntax allows) please reformat this block to include individual cases bulleted.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:126-127
+ A string which is can be used as an integer. The default value is ``""``.
+ If ``Default``, ``d`` or not set the checker rely on the existence/value of
+ the ``__STDC_WANT_LIB_EXT1__`` macro.
+ If ``Yes``, ``y`` or non-zero value the target environment implements ``_s``
----------------
or ~~not set~~ empty string (default value), the checker rel**ies** on the `__STDC_WANT_LIB_EXT1__` macro being defined.
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:128-130
+ If ``Yes``, ``y`` or non-zero value the target environment implements ``_s``
+ suffixed memory and character handler functions which are safer than older
+ version (e.g. ``memcpy_s()``).
----------------
~~value~~
the target environment **is considered to** implement~~s~~
"Character handler" -- did you mean "string transformation" or "string handling"?
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:131-132
+ version (e.g. ``memcpy_s()``).
+ If ``No``, ``n`` or zero value the ``_s`` suffixed functions are not
+ available.
+
----------------
`No`, `n`, or `0` (zero), the ...
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:134
+
+.. option:: InjectUL
+
----------------
If this is a user-facing thing, I would like to find a better name for this. Why and when are unsigned long increments needed?
https://reviews.llvm.org/D45050
More information about the cfe-commits
mailing list