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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits