[PATCH] D74877: [clang] fix incorrect Wdocumentation fix-its

[Mark de Wever via Phabricator via llvm-commits]

Mordante added a comment.

I think the patch looks better, but I still would like to get @gribozavr2 's opinion.

I'm not convinced the new approach is better in all cases, so I'm somewhat concerned about regressions from the current behaviour. Did you test this patch on a codebase to see the difference between the previous and new behaviour?

For the next time when providing a patch, please do the cleanups in a separate patch, that makes reviewing the real changes a bit easier.



================
Comment at: clang/lib/AST/CommentSema.cpp:22
+
+#define DEBUG_TYPE "commentsema"
 
----------------
Is the debug output really that interesting after the code has been committed?


================
Comment at: clang/lib/AST/CommentSema.cpp:756
+                            ArrayRef<const ParmVarDecl *> ParamVars,
+                            const bool IsFunctionOrMethodVariadic) {
+  SmallVector<const ParamCommandComment *, 8> PCCs;
----------------
In general Clang doesn't use top level `const` often, please remove them from the function argument.


================
Comment at: clang/lib/AST/CommentSema.cpp:759
+
+  for (BlockContentComment *const Block : FC->getBlocks()) {
+    ParamCommandComment *const PCC = dyn_cast<ParamCommandComment>(Block);
----------------
Please use `auto *` in range-based for loops.


================
Comment at: clang/lib/AST/CommentSema.cpp:760
+  for (BlockContentComment *const Block : FC->getBlocks()) {
+    ParamCommandComment *const PCC = dyn_cast<ParamCommandComment>(Block);
+    if (PCC && PCC->hasParamName()) {
----------------
Please use `auto *` since the type is already on the right hand side.


================
Comment at: clang/lib/AST/CommentSema.cpp:779
+/// mapping issues (no matching ParmVar or duplicate Comment).
+static SmallVector<const ParmVarDecl *, 8> mapParamCommentsToParmVars(
+    const SmallVector<const ParamCommandComment *, 8> &PCCs,
----------------
I would suggest to rename the function. The current name doesn't match what the function does. For example `findUndocumentedParams`.


================
Comment at: clang/lib/AST/CommentSema.cpp:787
+  // VarArg.
+  SmallVector<const ParamCommandComment *, 8> ParmVarDeclToParamCommandComment(
+      ParmVars.size(), nullptr);
----------------
I would suggest to make the name of the variable it bit shorter to improve readability.


================
Comment at: clang/lib/AST/CommentSema.cpp:790
+
+  for (const ParamCommandComment *const PCC : PCCs) {
+    if (PCC->isVarArgParam()) {
----------------
Please use `const auto *`


================
Comment at: clang/lib/AST/CommentSema.cpp:828
+
+  for (const ParamCommandComment *const PCC : ParamComments)
+    if (!PCC->isParamIndexValid())
----------------
Please use `const auto*`


================
Comment at: clang/lib/AST/CommentSema.cpp:859
+  else {
+    for (unsigned I = 0, E = UnresolvedParamComments.size(); I != E; ++I) {
+
----------------
Please reduce the number of blank lines in the hunk below.


================
Comment at: clang/lib/AST/CommentSema.cpp:873
+          // parameter! Remove old and new correction.
+          LLVM_DEBUG(llvm::dbgs() << "Duplicate suggestion to correct @"
+                                  << ParamName << " to "
----------------
I'm not sure whether that's the best behaviour. Can't we test which of the two suggestions is the best and then keep the suggestion there?


================
Comment at: clang/lib/AST/CommentSema.cpp:917
+    return;
   }
 
----------------
Please don't use braces on if's with one substatement. Maybe move the comment before the if.


================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:81
+/// \param ArgV Almost matches.
+int test2to2_bothAlmost(int argc, int argv);
+
----------------
Here I actually would like to get the fixit. No longer showing the `argc` when typing `ArgC` feels not good to me.


================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:91
+/// \param ZZZZZZ Does not match at all.
+int test2to2_oneAlmost(int argc, int argv);
+
----------------
Here there's one undocumented parameter left and one `\param` why not match them?


================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:96
+// expected-warning at +4 {{parameter 'ArgC' not found in the function declaration}} expected-note at +4 {{did you mean}}
+// expected-warning-NOT at +4 {{parameter 'ZZZZZZ' not found in the function declaration}} expected-note-NOT at +4 {{did you mean}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:
----------------
why test for ZZZZZZ ?


================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:137
+
+
+
----------------
Please reduce the amount of whitespace.


================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:149
+  */
+void run(const char *const arg[], int argc);
+
----------------
Here I would expect the `cnt` to be matched with `argc`.


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

https://reviews.llvm.org/D74877