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

[Alexander Lanin via Phabricator via llvm-commits]

AlexanderLanin marked 3 inline comments as done.
AlexanderLanin added inline comments.


================
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
----------------
Mordante wrote:
> 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.
I didn't touch this section that much. I extracted one of the ifs from the loop, but apparantely not the other... anyway I can certainly fix this.


================
Comment at: clang/lib/AST/CommentSema.cpp:757
+      const ParmVarDecl *CorrectedPVD = OrphanedParamDecls[CorrectedParamIndex];
+      Mapped[i] = CorrectedPVD;
+    }
----------------
Mordante wrote:
> 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.
I fully agree on that. However I went too far with cleanup and now I'm not quite sure what to do. To avoid adding another parallel review here, I'll just point you to https://github.com/AlexanderLanin/llvm-project/commit/d043f49089ebe3b0d1b4d8df02fbd0b3985b4d18 if thats fine? Just to figure out whether to abandon this change as it is here and switch to the other one or not. I ended up reducing resolveParamCommandIndexes to less then a dozen (hopefully readable) lines. Adding and removing is resolved with a MappedCounter, since other approaches failed to recognize triplicates.
If that looks fine in general I would replace this review. I personally like the smaller understandable functional methods, if that doesn't violate the general rules/ideas where the code should go.  Otherwise I'll apply your findings here.


================
Comment at: llvm/unittests/ADT/StringRefTest.cpp:536
+  EXPECT_EQ(2U, Hello.edit_distance("hellooo"));
+  EXPECT_EQ(2U, Hello.edit_distance("hel"));
 
----------------
Mordante wrote:
> I don't mind this, but was it intentionally?
I know it's slightly off topic, but not quite worth it's own commit. I was trying to figure out why 'ThisOneHasBeenRemovedButIsStillDocumented' was auto fixed to 'aaa' and my first guess was edit_distance doesn't correctly handle removed chars. As the code itself is not really readable for mere mortals and such a test was missing I added it here.

Turns out it was a special rule for `if only one \\param, then don't check edit_distance`. This is now slightly modified into `if only one \\param and only one ParmVarDecl, then don't check edit_distance`


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

https://reviews.llvm.org/D74877