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

[Mark de Wever via Phabricator via llvm-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.


================
Comment at: clang/lib/AST/CommentSema.cpp:757
+      const ParmVarDecl *CorrectedPVD = OrphanedParamDecls[CorrectedParamIndex];
+      Mapped[i] = CorrectedPVD;
+    }
----------------
AlexanderLanin wrote:
> 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.
I think it makes sense to abandon this review and start a new one. As far as I know we don't have a policy regarding the size of functions. Personally I prefer smaller functions. Before putting up the new review, please address the comments in this review. Please have a look at coding standard https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable regarding the usage of `auto`.

I'm still not really fond of the algorithm used to remove the duplicates. I'll give it some thought to see whether I can come up with a better suggestion.




================
Comment at: llvm/unittests/ADT/StringRefTest.cpp:536
+  EXPECT_EQ(2U, Hello.edit_distance("hellooo"));
+  EXPECT_EQ(2U, Hello.edit_distance("hel"));
 
----------------
AlexanderLanin wrote:
> 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`
Oke, just wanted to make sure it was intentionally.


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

https://reviews.llvm.org/D74877