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

[Mark de Wever via Phabricator via llvm-commits]

Mordante added a reviewer: gribozavr2.
Mordante added a subscriber: gribozavr2.
Mordante added a comment.

Thanks for your patch. I also would like to get better fixits for the arguments.

I would also like to hear @gribozavr2's input.



================
Comment at: clang/lib/AST/CommentSema.cpp:733
+    const SmallVector<const ParmVarDecl *, 8> &OrphanedParamDecls) {
+  SmallVector<const ParmVarDecl *, 8> Mapped(UnresolvedParamCommands.size(),
+                                             nullptr);
----------------
I don't feel the name `Mapped` conveys what the variable does. IMO `Corrections` would be a better name.


================
Comment at: clang/lib/AST/CommentSema.cpp:746
+    unsigned CorrectedParamIndex = ParamCommandComment::InvalidParamIndex;
+    if (OrphanedParamDecls.size() == 1 && UnresolvedParamCommands.size() == 1) {
+      // If one parameter is not documented then that parameter is the only
----------------
Please don't use curly braces for single line `if` and `else` statements (also at other locations).
It makes no sense to test this condition every iteration. Please move this test out of the loop.


================
Comment at: clang/lib/AST/CommentSema.cpp:757
+      const ParmVarDecl *CorrectedPVD = OrphanedParamDecls[CorrectedParamIndex];
+      Mapped[i] = CorrectedPVD;
+    }
----------------
I don't like adding duplicates here, and removed them in `setDuplicatesToNullptr`. This is inefficient.
Maybe add a mapping between `CorrectedParamIndex` and the first position in `Mapped`. Then when a duplicate is found the first position can be set to `nullptr` and the new position not added.


================
Comment at: clang/lib/AST/CommentSema.cpp:861
 
-  // Second pass over unresolved \\param commands: do typo correction.
-  // Suggest corrections from a set of parameter declarations that have no
-  // corresponding \\param.
-  for (unsigned i = 0, e = UnresolvedParamCommands.size(); i != e; ++i) {
-    const ParamCommandComment *PCC = UnresolvedParamCommands[i];
-
-    SourceRange ArgRange = PCC->getParamNameRange();
-    StringRef ParamName = PCC->getParamNameAsWritten();
-    Diag(ArgRange.getBegin(), diag::warn_doc_param_not_found)
-      << ParamName << ArgRange;
-
-    // All parameters documented -- can't suggest a correction.
-    if (OrphanedParamDecls.size() == 0)
-      continue;
-
-    unsigned CorrectedParamIndex = ParamCommandComment::InvalidParamIndex;
-    if (OrphanedParamDecls.size() == 1) {
-      // If one parameter is not documented then that parameter is the only
-      // possible suggestion.
-      CorrectedParamIndex = 0;
-    } else {
-      // Do typo correction.
-      CorrectedParamIndex = correctTypoInParmVarReference(ParamName,
-                                                          OrphanedParamDecls);
-    }
-    if (CorrectedParamIndex != ParamCommandComment::InvalidParamIndex) {
-      const ParmVarDecl *CorrectedPVD = OrphanedParamDecls[CorrectedParamIndex];
-      if (const IdentifierInfo *CorrectedII = CorrectedPVD->getIdentifier())
-        Diag(ArgRange.getBegin(), diag::note_doc_param_name_suggestion)
-          << CorrectedII->getName()
-          << FixItHint::CreateReplacement(ArgRange, CorrectedII->getName());
-    }
-  }
+  auto mapping = createUnresolvedParamMappingToOrphanedParams(
+      UnresolvedParamCommands, OrphanedParamDecls);
----------------
Please capitalize `mapping`.
I don't feel the name `mapping` conveys what the variable does. IMO `Corrections` would be a better name.
Please don't use `auto` when the type is not clear.


================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:37
+
+// Auto-fixing those two parameters would potentially worthen the situation as information is lost.
+// At the moment they at least have different names and a human can resonably pick the correct one by name.
----------------
worthen -> worsen


================
Comment at: llvm/unittests/ADT/StringRefTest.cpp:536
+  EXPECT_EQ(2U, Hello.edit_distance("hellooo"));
+  EXPECT_EQ(2U, Hello.edit_distance("hel"));
 
----------------
I don't mind this, but was it intentionally?


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

https://reviews.llvm.org/D74877