[clang] fix incorrect Wdocumentation fix-its

[AlexanderLanin]

Summary

Wdocumentation no longer provides fix-its which map multiple param comments to the same parameter.

This commit fixes:

• https://bugs.llvm.org/show_bug.cgi?id=43755
• https://bugs.llvm.org/show_bug.cgi?id=43808

[Mordante]

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

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

clang/lib/AST/CommentSema.cpp

736

I don't feel the name Mapped conveys what the variable does. IMO Corrections would be a better name.

749

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.

760

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.

898–899

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.

clang/test/Sema/warn-documentation-fixits.cpp

35

worthen -> worsen

llvm/unittests/ADT/StringRefTest.cpp

539

I don't mind this, but was it intentionally?

[AlexanderLanin]

clang/lib/AST/CommentSema.cpp

749

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.

760

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.

llvm/unittests/ADT/StringRefTest.cpp

539

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

[Mordante]

clang/lib/AST/CommentSema.cpp

760

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.

llvm/unittests/ADT/StringRefTest.cpp

539

Oke, just wanted to make sure it was intentionally.

[AlexanderLanin]

Slightly more code changed, assuming it's better understandable now. Review findings incorporated.

[Mordante]

@gribozavr2 what's your opinion of this patch?

[AlexanderLanin]

*ping*

[Mordante]

Sorry it took a while.

Do you want to continue this patch or switch to the other one as you mentioned before?

Some other remarks:

• In the tests please include the exact issues mentioned in PR43755 and PR43808.
• I'd also like the patch to be a bit more robust regarding the fix-its. @gribozavr2 also offered some suggestions:
  • Take all \params and function parameters in account when looking for a solution and find the best total solution.
  • If the edit distance is too large don't use that suggestion.
  • Since most people tend to document \params in the same order as the function parameters you can also take that into account.

Can you add a few more complex test cases, for example something like:

/**
 * \param aabb X
 * \param abbb Y
 */
void foo(int NotMe, int bbbb);

Which comment will be attached to bbbb?

[AlexanderLanin]

Thanks for the feedback!!

Do you want to continue this patch or switch to the other one as you mentioned before?

I had already updated this pull request to incorporate improvements from the github link. Sorry I didn't make that clear.
It's doesn't have those very small functions. That is in order to keep most of the not so easy understandable mapping vectors in small local scope.
Also to avoid some redundancy like adding all mappings and then removing the duplicates.

In the tests please include the exact issues mentioned in PR43755 and PR43808.

Both reports are from me and I'm quite certain I have them covered in tests. I will add a reference to the exact test cases.

Take all \params and function parameters in account when looking for a solution and find the best total solution.

Currently all unmapped parameters are taken into account.
I cannot imagine a use case where taking the mapped ones into account could influence the outcome.
Also I personally don't like breaking any existing correct documentation.
Do you have some example?

If the edit distance is too large don't use that suggestion.

Shall I remove the 1:1 exception which disregards edit distance?
The one from fixSinceThereIsOnlyOneOption?
I tried to not modify any not bug related behavior and this was there before.

Since most people tend to document \params in the same order as the function parameters you can also take that into account.

Sounds reasonable, but it might not be trivial to find an appropriate heuristic weighting this against edit distance.
I would like to exclude it from this patch and attempt that in the next one.

Can you add a few more complex test cases, for example something like:

Sure! I missed that one. I'll add some more.

[AlexanderLanin]

More tests added to prevent further regressions. It's quite difficult to figure out tests which might fail, but now there is a whole bunch of them so it looks good.
Added explicit references from tests to the two bug reports.

If there is anything else I should fix, I'd gladly do so!

If there is nothing else please commit it as I cannot.
Alexander Lanin <[e-mail]>

[AlexanderLanin]

*ping*

[Mordante]

Take all \params and function parameters in account when looking for a solution and find the best total solution.

Currently all unmapped parameters are taken into account.
I cannot imagine a use case where taking the mapped ones into account could influence the outcome.
Also I personally don't like breaking any existing correct documentation.
Do you have some example?

For example what happens in this case:

/**
 * \param aaaa Should match aaa
 * \param aaab  Should match aab
 * \param aaac  Should match aac
 */
void foo(int aaa, int aab, int aac);

And in the almost similar case:

/**
 * \param aaaa Should match aaa
 * \param aaab  Should match aab
 * \param aaac  Should match aac
 */
void foo(int aac, int aab, int aaa);

Does it give the same final result or does the order of the comments and parameters influence the outcome?

I would like to have a closer look at the algorithm you're using, I hope to find time for that later this week.

clang/lib/AST/CommentSema.cpp

735

Please capitalize the i and e.

783

Since the auto type is a pointer, please use a * to make it clear.

789

Please don't use auto unless the type is clear.

794

Is it intended that the assignment ParmVarDeclToParamCommandComment[PCC->getParamIndex()] = PCC; is executed? If so please add some comment.

804–806

Please capitalize the i and e.

816

Since the auto type is a pointer, please use a * to make it clear.

846

Please capitalize the i and e.

866

Please capitalize the i and e.

clang/test/Sema/warn-documentation-fixits.cpp

74

Why use a different test case than in the bug report? I really would like to see this patch fixes that case.

[AlexanderLanin]

I need to have a look why the mentioned test case is failing. I'll also add the two examples you provided to the next upload.

clang/lib/AST/CommentSema.cpp

735

Sorry such stuff should not require a manual review. I fixed this and similar issues by running clang-tidy --checks="readability-identifier-naming" -p=/home/alex/llvm/build CommentSema.cpp --fix

clang/test/Sema/warn-documentation-fixits.cpp

74

You are totally right. Added test case; It's failing; needs analysis. I thought I had that fixed. Will check ASAP.

[AlexanderLanin]

• Add test cases exactly as in bug reports.
• Test case from https://bugs.llvm.org/show_bug.cgi?id=43808 triggered a second bug due to the short parameter names used. Removed incorrect quick exit in SimpleTypoCorrector (MinPossibleEditDistance).
• The two requested cases don't work since 'aaab' is as close to 'aaa' as it is to 'aab' (one char). Nothing is changed as there is no best match available. Instead two new test cases with aaab, aaac and aaad added (test3to3_allAlmost + test3to3_allAlmost_rev).

[AlexanderLanin]

*ping*

[AlexanderLanin]

*ping*

[AlexanderLanin]

*ping*
Last change was over 3 months ago. Should this be merged or abandoned?
It doesn't solve everything. It doesn't improve everything. But it's a fix to two specific bugs.
Is someone missing as a reviewer to get this accepted/rejected any faster?

[AlexanderLanin]

Rebased and stored at https://github.com/AlexanderLanin/llvm-project/tree/Wdocumentation-fix-its so I have a branch next time I need to rebase :-)

Still looking for someone to approve and to commit this change.

[AlexanderLanin]

Added one missing expect statement in this revision.

Could someone please review and optionally approve and to commit this change?
That would be great!
It's been open for a while now ;-)

[Mordante]

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.

clang/lib/AST/CommentSema.cpp

22

Is the debug output really that interesting after the code has been committed?

771

In general Clang doesn't use top level const often, please remove them from the function argument.

774

Please use auto * in range-based for loops.

775

Please use auto * since the type is already on the right hand side.

804

I would suggest to rename the function. The current name doesn't match what the function does. For example findUndocumentedParams.

812

I would suggest to make the name of the variable it bit shorter to improve readability.

815

Please use const auto *

820

Please use const auto*

851

Please reduce the number of blank lines in the hunk below.

865

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?

886

Please don't use braces on if's with one substatement. Maybe move the comment before the if.

clang/test/Sema/warn-documentation-fixits.cpp

79

Here I actually would like to get the fixit. No longer showing the argc when typing ArgC feels not good to me.

89

Here there's one undocumented parameter left and one \param why not match them?

94

why test for ZZZZZZ ?

135

Please reduce the amount of whitespace.

147

Here I would expect the cnt to be matched with argc.

[AlexanderLanin]

clang/lib/AST/CommentSema.cpp

22

There are a few debug print statements left in code below. They are certainly not interesting until someone tries to debug the code ;-)
Shall I remove this line and those llvm::dbgs() statements?

771

Sorry I was not familiar with that convention. I've removed const which affects only the definition and not the declaration (e.g. I left in case of const ref).

774

Sure, I'll fix it! It even felt a little strange to me. I tried to follow the guidelines which say to skip the type if it's already obvious from context. I though the exact type is not obvious since it's not getBlockContentComment(). But it's close enough :-)

804

Thanks, that should have been obvious from the comment 'Returns all ParmVarDecl without a comment'.

865

Sounds like a very reasonable improvement idea.
However on first glance it would require quite some changes starting with SimpleTypoCorrector/correctTypoInParmVarReference even returning the BestEditDistance.
Would it make more sense to address them in a follow up patch?

clang/test/Sema/warn-documentation-fixits.cpp

79

They both have the same edit distance of two mismatching chars. There is no logic in StringRef::edit_distance which would say 'c' is closer to 'C' then it is to 'V'.
I guess that would be a nice follow up improvement e.g. via another flag to edit_distance bool CaseIsNotVeryImportant.
edit_distance is used by ~23 files and I guess several of them could benefit from that new flag.

Note: at the moment both are fixed to 'argc' which is worse then not fixing at all.

94

Copy paste error. There is a ZZZZZZ before and after this test case.

147

This is currently not detected. Here is the minimal scenario:

/// \param a This one is correctly documented.
/// \param c Total mismatch. Fix it since it's only one comment to only one parameter ?!
int oneUnmatchedCommentToOneParameter(int a, int b);

Unfortunately I disagree. Sorry. Auto fixing that would be a no go for me personally. Because in my personal experience it's a quite common use case that one parameter is removed (but not the documentation) and then a different *unrelated* parameter is introduced (with no documentation). Just moving the old documentation over to this new parameter would hide/create a significant problem. Without 'fixing' it's at least obvious that there is a problem there.
I've not encountered this problem in real life with only one parameter, so I'm ok with fixSinceThereIsOnlyOneOption behavior.

[AlexanderLanin]

clang/test/Sema/warn-documentation-fixits.cpp

147

Ok, that's not what is implemented: It does get fixed. So I was contemplating on what that means. Any code change, including or especially the automatic ones, do go through a code review. So it 'should' be ok to 'fix' stuff. That's what we have reviews for anyway.
I'm not too happy about it due to reasons above, but ok.

In this case it doesn't get suggested/fixed immediately on the first pass. Only after arg[] is fixed to arg it will be detected on subsequent compilation. It's not ideal, but good enough?!

[AlexanderLanin]

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?

There are no warnings in llvm or ccache, all I found is a low quality codebase at work which does produce 1007 unique Wdocumentation warnings (see wc command).

It *seems* I can run this via:

mkdir -p build-clang-D74877 && cd build-clang-D74877
CC=/home/alex/llvm/build-D74877/bin/clang CXX=/home/alex/llvm/build-D74877/bin/clang++ cmake .. -DCMAKE_CXX_FLAGS="-Wdocumentation -Wno-documentation-deprecated-sync" -GNinja
CC=/home/alex/llvm/build-D74877/bin/clang CXX=/home/alex/llvm/build-D74877/bin/clang++ cmake .. -DCMAKE_CXX_FLAGS="-Wdocumentation -Wno-documentation-deprecated-sync -fsyntax-only" -GNinja
ninja -j1 -k10000 > warnings.txt
grep "\[-Wdocumentation\]" warnings.txt | grep -v "empty paragraph passed to" | grep -v "^\[" | sort | uniq | wc -l
cd ..

mkdir -p build-clang-master && cd build-clang-master
CC=/home/alex/llvm/build-master/bin/clang CXX=/home/alex/llvm/build-master/bin/clang++ cmake .. -DCMAKE_CXX_FLAGS="-Wdocumentation -Wno-documentation-deprecated-sync" -GNinja
CC=/home/alex/llvm/build-master/bin/clang CXX=/home/alex/llvm/build-master/bin/clang++ cmake .. -DCMAKE_CXX_FLAGS="-Wdocumentation -Wno-documentation-deprecated-sync -fsyntax-only" -GNinja
ninja -j1 -k10000 > warnings.txt
grep "\[-Wdocumentation\]" warnings.txt | grep -v "empty paragraph passed to" | grep -v "^\[" | sort | uniq | wc -l
cd ..

# .. in order to leave git
grep -v "^\[" build-clang-master/warnings.txt > ../clang-master-warnings.txt
grep -v "^\[" build-clang-D74877/warnings.txt > ../clang-D74877-warnings.txt

git diff ../clang-master-warnings.txt ../clang-D74877-warnings.txt

I do not know how to apply suggested fixes for an easier diff.
I could run clang-tidy with some inappropriate check, so that it will fix all the Wdocumentation warnings only. But that takes forever.

Anyway based on the approach above I found two additional regressions. Testing with real code seems to be kind of important ;-)

• Reported column numbers have been changed. Apparently this cannot be checked by expected-warning and expected-note, so I added another FileCheck pass with a few select test cases.
• All warnings are reported first and then all the notes. Apparently this is not checked by expected-warning and expected-note. This will probably also require to be checked by FileCheck directly.

I'll further analyze the diff ASAP (have to fix the second problem first so that the diff gets smaller).
But that experience was kind of enough to get the following idea:
I would like to create two different patches:

• One with all the new tests (modified to pass based on current code). It should be trivial to review.
• One with the new code and the required small modifications to tests.

Because at the moment lots of new test cases are added and it's not clear what is even different now with this patch.
That seems to be the best way to ensure all changes are clearly visible within test cases.

What do you think of that @Mordante?

[Mordante]

I do not know how to apply suggested fixes for an easier diff.
I could run clang-tidy with some inappropriate check, so that it will fix all the Wdocumentation warnings only. But that takes forever.

Would it be feasible to run it on a part of the clang codebase just to get a feeling, or does that also take too long?

Anyway based on the approach above I found two additional regressions. Testing with real code seems to be kind of important ;-)

• Reported column numbers have been changed. Apparently this cannot be checked by expected-warning and expected-note, so I added another FileCheck pass with a few select test cases.

Maybe first look why the columns have changed and whether it's a real issue.

• All warnings are reported first and then all the notes. Apparently this is not checked by expected-warning and expected-note. This will probably also require to be checked by FileCheck directly.

AFAIK other diagnostic tests don't test the in which the diagnostics are emitted. So I wonder whether this is really required.

I'll further analyze the diff ASAP (have to fix the second problem first so that the diff gets smaller).
But that experience was kind of enough to get the following idea:
I would like to create two different patches:

• One with all the new tests (modified to pass based on current code). It should be trivial to review.
• One with the new code and the required small modifications to tests.

I think if would be first good to figure out why this changes and then see whether it's a real issue. If it's an issue it needs to be fixed, else the separate patches would be preferred.

clang/lib/AST/CommentSema.cpp

22

Yes please. When needed again for debugging it's trivial to add the wanted debug statements.

865

Yes if it's not trivial a separate patch would be preferable.

clang/test/Sema/warn-documentation-fixits.cpp

79

Yes maybe if the edit distance is the same it would be a good idea to use the case insensitive match. But I agree this can be done in a followup patch.

147

Would it be easy to only show the note on the first pass without the fixit? Then the user still gets a hint, but not the fixit.

[AlexanderLanin]

Anyway based on the approach above I found two additional regressions. Testing with real code seems to be kind of important ;-)

• Reported column numbers have been changed. Apparently this cannot be checked by expected-warning and expected-note, so I added another FileCheck pass with a few select test cases.

Maybe first look why the columns have changed and whether it's a real issue.

• All warnings are reported first and then all the notes. Apparently this is not checked by expected-warning and expected-note. This will probably also require to be checked by FileCheck directly.

AFAIK other diagnostic tests don't test the in which the diagnostics are emitted. So I wonder whether this is really required.

I think if would be first good to figure out why this changes and then see whether it's a real issue. If it's an issue it needs to be fixed, else the separate patches would be preferred.

I think those unintentional changes are an issue. See where ^ points and how the readability of the messages decreases due to their order:

TimeFormatTypes.h:171:14: warning: parameter 'paramGpsWeek:' not found in the function declaration [-Wdocumentation]
        /// @param paramGpsWeek: weeks till 6.1.1980
             ^     ~~~~~~~~~~~~~
TimeFormatTypes.h:172:14: warning: parameter 'paramGpsTimeOfWeek:' not found in the function declaration [-Wdocumentation]
        /// @param paramGpsTimeOfWeek: seconds in current week
             ^     ~~~~~~~~~~~~~~~~~~~
TimeFormatTypes.h:173:14: warning: parameter 'paramLeapSeconds:' not found in the function declaration [-Wdocumentation]
        /// @param paramLeapSeconds: leap seconds, default 0
             ^     ~~~~~~~~~~~~~~~~~
TimeFormatTypes.h:171:14: note: did you mean 'paramGpsWeek'?
        /// @param paramGpsWeek: weeks till 6.1.1980
             ^     ~~~~~~~~~~~~~
                   paramGpsWeek
TimeFormatTypes.h:172:14: note: did you mean 'paramGpsTimeOfWeek'?
        /// @param paramGpsTimeOfWeek: seconds in current week
             ^     ~~~~~~~~~~~~~~~~~~~
                   paramGpsTimeOfWeek
TimeFormatTypes.h:173:14: note: did you mean 'paramLeapSeconds'?
        /// @param paramLeapSeconds: leap seconds, default 0
             ^     ~~~~~~~~~~~~~~~~~
                   paramLeapSeconds

I'll fix this shortly.

clang/test/Sema/warn-documentation-fixits.cpp

89

Because at the time this is executed there are technically two parameters unmatched. One has a fix-it suggestion.

147

Love the idea, will look into it!

clang/include/clang/AST/CommentSema.h

public:
   void inspectThisDecl();
 
   /// Returns index of a function parameter with a given name.
   unsigned resolveParmVarReference(StringRef Name,
                                    ArrayRef<const ParmVarDecl *> ParamVars);
 
   /// Returns index of a function parameter with the name closest to a given
   /// typo.
-  unsigned correctTypoInParmVarReference(StringRef Typo,
-                                         ArrayRef<const ParmVarDecl *> ParamVars);
+  static unsigned
+  correctTypoInParmVarReference(StringRef Typo,
 
   bool resolveTParamReference(StringRef Name,
                               const TemplateParameterList *TemplateParameters,
                               SmallVectorImpl<unsigned> *Position);
 
   StringRef correctTypoInTParamReference(
                               StringRef Typo,
                               const TemplateParameterList *TemplateParameters);

clang/lib/AST/CommentSema.cpp

 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringSwitch.h"
 
 namespace clang {
 namespace comments {
 
 namespace {
 #include "clang/AST/CommentHTMLTagsProperties.inc"
 } // end anonymous namespace
 
 Sema::Sema(llvm::BumpPtrAllocator &Allocator, const SourceManager &SourceMgr,
            DiagnosticsEngine &Diags, CommandTraits &Traits,
            const Preprocessor *PP) :
if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
     SmallString<64> TextToInsert = AttributeSpelling;
     TextToInsert += " ";
     SourceLocation Loc = FD->getSourceRange().getBegin();
     Diag(Loc, diag::note_add_deprecation_attr)
         << FixItHint::CreateInsertion(Loc, TextToInsert);
   }
 }
 
-void Sema::resolveParamCommandIndexes(const FullComment *FC) {
-  if (!isFunctionDecl()) {
-    // We already warned that \\param commands are not attached to a function
-    // decl.
-    return;
-  }
+static unsigned resolveParmVarReference(StringRef Name,
+                                        ArrayRef<const ParmVarDecl *> ParamVars,
+                                        bool IsFunctionOrMethodVariadic) {
+  for (unsigned i = 0, e = ParamVars.size(); i != e; ++i) {
+    const IdentifierInfo *II = ParamVars[i]->getIdentifier();
+    if (II && II->getName() == Name)
+      return i;
+  }
+  if (Name == "..." && IsFunctionOrMethodVariadic)
+    return ParamCommandComment::VarArgParamIndex;
+  return ParamCommandComment::InvalidParamIndex;
 
-  SmallVector<ParamCommandComment *, 8> UnresolvedParamCommands;
+// Extracts all ParamCommandComments while setting IsVarArgParam and ParamIndex
+static SmallVector<const ParamCommandComment *, 8>
+extractParamCommandComments(const FullComment *const FC,
+                            ArrayRef<const ParmVarDecl *> ParamVars,
+                            const bool IsFunctionOrMethodVariadic) {
+  SmallVector<const ParamCommandComment *, 8> PCCs;
+
+  for (auto Block : FC->getBlocks()) {
+    ParamCommandComment *PCC = dyn_cast<ParamCommandComment>(Block);
+    if (PCC && PCC->hasParamName()) {
+      StringRef ParamName = PCC->getParamNameAsWritten();
+      const unsigned Index = resolveParmVarReference(
+          ParamName, ParamVars, IsFunctionOrMethodVariadic);
+      if (Index == ParamCommandComment::VarArgParamIndex)
+        PCC->setIsVarArgParam();
+      else if (Index != ParamCommandComment::InvalidParamIndex)
+        PCC->setParamIndex(Index);
 
-  // Comment AST nodes that correspond to \c ParamVars for which we have
-  // found a \\param command or NULL if no documentation was found so far.
-  SmallVector<ParamCommandComment *, 8> ParamVarDocs;
+      PCCs.push_back(PCC);
+    }
+  }
 
-  ArrayRef<const ParmVarDecl *> ParamVars = getParamVars();
-  ParamVarDocs.resize(ParamVars.size(), nullptr);
+  return PCCs;
+}
 
-  // First pass over all \\param commands: resolve all parameter names.
-  for (Comment::child_iterator I = FC->child_begin(), E = FC->child_end();
-       I != E; ++I) {
-    ParamCommandComment *PCC = dyn_cast<ParamCommandComment>(*I);
-    if (!PCC || !PCC->hasParamName())
-      continue;
-    StringRef ParamName = PCC->getParamNameAsWritten();
+/// Returns all ParmVarDecl without a comment. Diagnosis is emitted in case of
+/// mapping issues (no matching ParmVar or duplicate Comment).
+static SmallVector<const ParmVarDecl *, 8> mapParamCommentsToParmVars(
+    const SmallVector<const ParamCommandComment *, 8> &PCCs,
+    const ArrayRef<const ParmVarDecl *> &ParmVars, DiagnosticsEngine &Diags) {
+  // Matching ParamCommandComment by index of ParmVars.
+  // It's basically a map<ParmVarDecl, ParamCommandComment> in terms of
+  // readability, but vector is faster, as it usually contains only very few
+  // elements and the index is known on access. Downside is no support for
+  // VarArg.
+  SmallVector<const ParamCommandComment *, 8> ParmVarDeclToParamCommandComment(
+      ParmVars.size(), nullptr);
 
-    // Check that referenced parameter name is in the function decl.
-    const unsigned ResolvedParamIndex = resolveParmVarReference(ParamName,
-                                                                ParamVars);
-    if (ResolvedParamIndex == ParamCommandComment::VarArgParamIndex) {
-      PCC->setIsVarArgParam();
-      continue;
+  for (auto PCC : PCCs) {
+    if (PCC->isVarArgParam()) {
+      // No additional diagnostic supported yet.
+    } else if (PCC->isParamIndexValid()) {
+      assert(PCC->getParamIndex() < ParmVars.size() &&
+             "PCCs and ParmVars vectors do not match");
+      if (auto PrevPCC =
+              ParmVarDeclToParamCommandComment[PCC->getParamIndex()]) {
+        Diags.Report(PCC->getLocation(), diag::warn_doc_param_duplicate)
+            << PCC->getParamNameAsWritten() << PCC->getParamNameRange();
+        Diags.Report(PrevPCC->getLocation(), diag::note_doc_param_previous)
+            << PrevPCC->getParamNameRange();
       }
-    if (ResolvedParamIndex == ParamCommandComment::InvalidParamIndex) {
-      UnresolvedParamCommands.push_back(PCC);
-      continue;
-    }
-    PCC->setParamIndex(ResolvedParamIndex);
-    if (ParamVarDocs[ResolvedParamIndex]) {
-      SourceRange ArgRange = PCC->getParamNameRange();
-      Diag(ArgRange.getBegin(), diag::warn_doc_param_duplicate)
-        << ParamName << ArgRange;
-      ParamCommandComment *PrevCommand = ParamVarDocs[ResolvedParamIndex];
-      Diag(PrevCommand->getLocation(), diag::note_doc_param_previous)
-        << PrevCommand->getParamNameRange();
-    }
-    ParamVarDocs[ResolvedParamIndex] = PCC;
-  }
+      ParmVarDeclToParamCommandComment[PCC->getParamIndex()] = PCC;
+    } else
+      Diags.Report(PCC->getLocation(), diag::warn_doc_param_not_found)
+          << PCC->getParamNameAsWritten() << PCC->getParamNameRange();
+
+  SmallVector<const ParmVarDecl *, 8> ParmVarsWithoutComment;
+
+  for (unsigned i = 0, e = ParmVars.size(); i != e; ++i)
+    if (!ParmVarDeclToParamCommandComment[i])
+      ParmVarsWithoutComment.push_back(ParmVars[i]);
+
+  return ParmVarsWithoutComment;
 
-  // Find parameter declarations that have no corresponding \\param.
-  SmallVector<const ParmVarDecl *, 8> OrphanedParamDecls;
-  for (unsigned i = 0, e = ParamVarDocs.size(); i != e; ++i) {
-    if (!ParamVarDocs[i])
-      OrphanedParamDecls.push_back(ParamVars[i]);
-  }
+static SmallVector<const ParamCommandComment *, 8>
+extractUnresolvedParamComments(
+    const SmallVector<const ParamCommandComment *, 8> &ParamComments) {
+  SmallVector<const ParamCommandComment *, 8> UnresolvedParamComments;
+
+  for (auto PCC : ParamComments)
+    if (!PCC->isParamIndexValid())
+      UnresolvedParamComments.push_back(PCC);
+
+  return UnresolvedParamComments;
 
-  // 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];
+void attemptToMatchUnresolvedComments(
+    const SmallVector<const ParamCommandComment *, 8> &UnresolvedParamComments,
+    const SmallVector<const ParmVarDecl *, 8> &OrphanedParmVars,
+    DiagnosticsEngine &Diags) {
+  // Quick exit for the normal case where UnresolvedParamComments is empty.
+  if (UnresolvedParamComments.empty() || OrphanedParmVars.empty())
+    return;
 
-    SourceRange ArgRange = PCC->getParamNameRange();
-    StringRef ParamName = PCC->getParamNameAsWritten();
-    Diag(ArgRange.getBegin(), diag::warn_doc_param_not_found)
-      << ParamName << ArgRange;
+  // Corrected OrphanedParmVar by index of UnresolvedParamComments.
+  SmallVector<const ParmVarDecl *, 8> ParmVarForComment(
+      UnresolvedParamComments.size(), nullptr);
 
-    // All parameters documented -- can't suggest a correction.
-    if (OrphanedParamDecls.size() == 0)
-      continue;
+  // ParmVarForComment index by index of OrphanedParmVars.
+  // Note: Entries are never deleted and are therefore partially part of
+  // duplications.
+  SmallVector<unsigned, 8> CommentToParmVar(
+      OrphanedParmVars.size(), ParamCommandComment::InvalidParamIndex);
 
-    unsigned CorrectedParamIndex = ParamCommandComment::InvalidParamIndex;
-    if (OrphanedParamDecls.size() == 1) {
-      // If one parameter is not documented then that parameter is the only
-      // possible suggestion.
-      CorrectedParamIndex = 0;
+  if (UnresolvedParamComments.size() == 1 && OrphanedParmVars.size() == 1)
+    ParmVarForComment[0] = OrphanedParmVars.front();
+  else {
+    for (unsigned i = 0, e = UnresolvedParamComments.size(); i != e; ++i) {
+
+      StringRef ParamName = UnresolvedParamComments[i]->getParamNameAsWritten();
+
+      unsigned CorrectedParmVarIndex =
+          Sema::correctTypoInParmVarReference(ParamName, OrphanedParmVars);
+
+      if (CorrectedParmVarIndex != ParamCommandComment::InvalidParamIndex) {
+        if (CommentToParmVar[CorrectedParmVarIndex] !=
+            ParamCommandComment::InvalidParamIndex) {
+          ParmVarForComment[CommentToParmVar[CorrectedParmVarIndex]] = nullptr;
+          ParmVarForComment[i] = nullptr;
         } else {
-      // Do typo correction.
-      CorrectedParamIndex = correctTypoInParmVarReference(ParamName,
-                                                          OrphanedParamDecls);
+          CommentToParmVar[CorrectedParmVarIndex] = i;
+          ParmVarForComment[i] = OrphanedParmVars[CorrectedParmVarIndex];
+        }
       }
-    if (CorrectedParamIndex != ParamCommandComment::InvalidParamIndex) {
-      const ParmVarDecl *CorrectedPVD = OrphanedParamDecls[CorrectedParamIndex];
-      if (const IdentifierInfo *CorrectedII = CorrectedPVD->getIdentifier())
-        Diag(ArgRange.getBegin(), diag::note_doc_param_name_suggestion)
+    }
+  }
+
+  for (unsigned i = 0, e = UnresolvedParamComments.size(); i != e; ++i) {
+    if (ParmVarForComment[i]) {
+      if (const IdentifierInfo *CorrectedII =
+              ParmVarForComment[i]->getIdentifier()) {
+        const ParamCommandComment *PCC = UnresolvedParamComments[i];
+
+        Diags.Report(PCC->getLocation(), diag::note_doc_param_name_suggestion)
             << CorrectedII->getName()
-          << FixItHint::CreateReplacement(ArgRange, CorrectedII->getName());
+            << FixItHint::CreateReplacement(PCC->getParamNameRange(),
+                                            CorrectedII->getName());
+      }
     }
   }
 }
 
+void Sema::resolveParamCommandIndexes(const FullComment *FC) {
+  if (!isFunctionDecl()) {
+    // We already warned that \\param commands are not attached to a function
+    // decl.
+    return;
+  }
+
+  const ArrayRef<const ParmVarDecl *> ParmVars = getParamVars();
+  const SmallVector<const ParamCommandComment *, 8> ParamComments =
+      extractParamCommandComments(FC, ParmVars, isFunctionOrMethodVariadic());
+
+  const SmallVector<const ParmVarDecl *, 8> ParmVarsWithoutComment =
+      mapParamCommentsToParmVars(ParamComments, ParmVars, Diags);
+
+  const SmallVector<const ParamCommandComment *, 8> UnresolvedParamComments =
+      extractUnresolvedParamComments(ParamComments);
+
+  attemptToMatchUnresolvedComments(UnresolvedParamComments,
+                                   ParmVarsWithoutComment, Diags);
+}
+
 bool Sema::isFunctionDecl() {
   if (!ThisDeclInfo)
     return false;
   if (!ThisDeclInfo->IsFilled)
     inspectThisDecl();
   return ThisDeclInfo->getKind() == DeclInfo::FunctionKind;
 }
 

 }
 
 void Sema::inspectThisDecl() {
   ThisDeclInfo->fill();
 }
 
 unsigned Sema::resolveParmVarReference(StringRef Name,
                                        ArrayRef<const ParmVarDecl *> ParamVars) {
-  for (unsigned i = 0, e = ParamVars.size(); i != e; ++i) {
-    const IdentifierInfo *II = ParamVars[i]->getIdentifier();
-    if (II && II->getName() == Name)
-      return i;
-  }
-  if (Name == "..." && isFunctionOrMethodVariadic())
-    return ParamCommandComment::VarArgParamIndex;
-  return ParamCommandComment::InvalidParamIndex;
+  return ::clang::comments::resolveParmVarReference(
+      Name, ParamVars, isFunctionOrMethodVariadic());
 }
 
 namespace {
 class SimpleTypoCorrector {
   const NamedDecl *BestDecl;
 
   StringRef Typo;
   const unsigned MaxEditDistance;

clang/test/Sema/warn-documentation-fixits.cpp

 // RUN: %clang_cc1 -fsyntax-only -Wdocumentation -Wdocumentation-pedantic -fcomment-block-commands=foobar -verify %s
 // RUN  %clang_cc1 -std=c++11 -fsyntax-only -Wdocumentation -Wdocumentation-pedantic -fcomment-block-commands=foobar -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck -DATTRIBUTE="__attribute__((deprecated))" %s
 // RUN: %clang_cc1 -std=c++14 -fsyntax-only -Wdocumentation -Wdocumentation-pedantic -fcomment-block-commands=foobar -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck --check-prefixes=CHECK,CHECK14 -DATTRIBUTE="[[deprecated]]" %s
 
-// expected-warning@+1 {{parameter 'ZZZZZZZZZZ' not found in the function declaration}} expected-note@+1 {{did you mean 'a'?}}
+// expected-warning@+2 {{parameter 'ZZZZZZZZZZ' not found in the function declaration}} expected-note@+2 {{did you mean 'a'?}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+1]]:12-[[@LINE+1]]:22}:"a"
 /// \param ZZZZZZZZZZ Blah blah.
-int test1(int a);
+int fixSinceThereIsOnlyOneOption(int a);
 
-// expected-warning@+1 {{parameter 'aab' not found in the function declaration}} expected-note@+1 {{did you mean 'aaa'?}}
+// expected-warning@+2 {{parameter 'aab' not found in the function declaration}} expected-note@+2 {{did you mean 'aaa'?}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+1]]:12-[[@LINE+1]]:15}:"aaa"
 /// \param aab Blah blah.
-int test2(int aaa, int bbb);
+int fixParamCommentToBestMatch(int aaa, int bbb);
 
-// expected-warning@+1 {{template parameter 'ZZZZZZZZZZ' not found in the template declaration}} expected-note@+1 {{did you mean 'T'?}}
+// expected-warning@+3 {{parameter 'aab' not found in the function declaration}} expected-note-NOT@+3 {{did you mean}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+2]]:
+/// \param aaa Documented
+/// \param aab Do not duplicate documentation of aaa.
+int doNotDuplicateExistingComments(int aaa);
+
+// expected-warning@+4 {{parameter 'aaaa' not found in the function declaration}} expected-note@+4 {{did you mean 'aaa'?}}
+// expected-warning@+4 {{parameter 'ThisOneHasBeenRemovedButIsStillDocumented' not found in the function declaration}} expected-note-NOT@+4 {{did you mean}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+2]]:
+/// \param aaaa Almost there.
+/// \param ThisOneHasBeenRemovedButIsStillDocumented Widely different name!
+int doNotDuplicateCommentsViaFixIt(int aaa);
+
+// expected-warning@+6 {{parameter 'aaaa' not found in the function declaration}} expected-note-NOT@+6 {{did you mean}}
+// expected-warning@+6 {{parameter 'aaab' not found in the function declaration}} expected-note-NOT@+6 {{did you mean}}
+// expected-warning@+6 {{parameter 'aaac' not found in the function declaration}} expected-note-NOT@+6 {{did you mean}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+3]]:
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+3]]:
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+3]]:
+/// \param aaaa Almost there!
+/// \param aaab Me too!
+/// \param aaac Me three!
+int onlyFixWhenExactlyOneBestParamComment(int aaa);
+
+// Auto-fixing those two parameters would potentially worsen 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.
+// expected-warning@+4 {{parameter 'ArgC' not found in the function declaration}} expected-note-NOT@+4 {{did you mean}}
+// expected-warning@+4 {{parameter 'ArgV' not found in the function declaration}} expected-note-NOT@+4 {{did you mean}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+2]]:
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+2]]:
+/// \param ArgC Number of elements.
+/// \param ArgV Array of elements (int for simplicity here).
+int test2to2(int argc, int argv);
+
+// expected-warning@+2 {{template parameter 'ZZZZZZZZZZ' not found in the template declaration}} expected-note@+2 {{did you mean 'T'?}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+1]]:13-[[@LINE+1]]:23}:"T"
 /// \tparam ZZZZZZZZZZ Aaa
 template<typename T>
 void test3(T aaa);
 
-// expected-warning@+1 {{template parameter 'SomTy' not found in the template declaration}} expected-note@+1 {{did you mean 'SomeTy'?}}
+// expected-warning@+2 {{template parameter 'SomTy' not found in the template declaration}} expected-note@+2 {{did you mean 'SomeTy'?}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+1]]:13-[[@LINE+1]]:18}:"SomeTy"
 /// \tparam SomTy Aaa
 /// \tparam OtherTy Bbb
 template<typename SomeTy, typename OtherTy>
 void test4(SomeTy aaa, OtherTy bbb);
 
-// expected-warning@+1 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+// expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:1-[[@LINE+2]]:1}:"[[ATTRIBUTE]] "
 /// \deprecated
 void test_deprecated_1();
 
-// expected-warning@+1 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+// expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:1-[[@LINE+2]]:1}:"[[ATTRIBUTE]] "
 /// \deprecated
 void test_deprecated_2(int a);
 
 struct test_deprecated_3 {
-  // expected-warning@+1 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+  // expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:3-[[@LINE+2]]:3}:"[[ATTRIBUTE]] "
   /// \deprecated
   void test_deprecated_4();
 
-  // expected-warning@+1 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+  // expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:3-[[@LINE+2]]:3}:"[[ATTRIBUTE]] "
   /// \deprecated
   void test_deprecated_5() {
   }
 };
 
 template<typename T>
 struct test_deprecated_6 {
-  // expected-warning@+1 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+  // expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:3-[[@LINE+2]]:3}:"[[ATTRIBUTE]] "
   /// \deprecated
   void test_deprecated_7();
 
-  // expected-warning@+1 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+  // expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:3-[[@LINE+2]]:3}:"[[ATTRIBUTE]] "
   /// \deprecated
   void test_deprecated_8() {
   }
 };
 
 class PR43753 {
-  // expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}}
-  // expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+  // expected-warning@+3 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}}
+  // expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:3-[[@LINE+2]]:3}:"[[ATTRIBUTE]] "
   /// \deprecated
   static void test_deprecated_static();
 
-  // expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}}
-  // expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+  // expected-warning@+3 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}}
+  // expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:3-[[@LINE+2]]:3}:"[[ATTRIBUTE]] "
   /// \deprecated
   static auto test_deprecated_static_trailing_return() -> int;
 
 #if __cplusplus >= 201402L
   // expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}}
   // expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
   /// \deprecated
   static decltype(auto) test_deprecated_static_decltype_auto() { return 1; }
 #endif
 
   // expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}}
   // expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
   /// \deprecated
   void test_deprecated_const() const;
 
   // expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}}
   // expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
   /// \deprecated
   auto test_deprecated_trailing_return() -> int;
 
 #if __cplusplus >= 201402L
-  // expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}}
-  // expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+  // expected-warning@+3 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}}
+  // expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+  // CHECK14: fix-it:"{{.*}}":{[[@LINE+2]]:3-[[@LINE+2]]:3}:"[[ATTRIBUTE]] "
   /// \deprecated
   decltype(auto) test_deprecated_decltype_auto() const { return a; }
 
 private:
   int a{0};
 #endif
 };
 #define MY_ATTR_DEPRECATED __attribute__((deprecated))
 
-// expected-warning@+1 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+// expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:1-[[@LINE+2]]:1}:"MY_ATTR_DEPRECATED "
 /// \deprecated
 void test_deprecated_9(int a);
 
 #if __cplusplus >= 201402L
 #define ATTRIBUTE_DEPRECATED [[deprecated]]
 
-// expected-warning@+1 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+2 {{add a deprecation attribute to the declaration to silence this warning}}
+// expected-warning@+2 {{declaration is marked with '\deprecated' command but does not have a deprecation attribute}} expected-note@+3 {{add a deprecation attribute to the declaration to silence this warning}}
+// CHECK14: fix-it:"{{.*}}":{[[@LINE+2]]:1-[[@LINE+2]]:1}:"ATTRIBUTE_DEPRECATED "
 /// \deprecated
 void test_deprecated_10(int a);
 #endif
 
 // rdar://12381408
-// expected-warning@+2  {{unknown command tag name 'retur'; did you mean 'return'?}}
+// expected-warning@+3  {{unknown command tag name 'retur'; did you mean 'return'?}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+2]]:6-[[@LINE+2]]:11}:"return"
 /// \brief testing fixit
 /// \retur int in FooBar
 int FooBar();
 
-// expected-warning@+1  {{unknown command tag name 'fooba'; did you mean 'foobar'?}}
+// expected-warning@+2  {{unknown command tag name 'fooba'; did you mean 'foobar'?}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+1]]:6-[[@LINE+1]]:11}:"foobar"
 /// \fooba bbb IS_DOXYGEN_END
 int gorf();
 
 // expected-warning@+1 {{unknown command tag name}}
 /// \t bbb IS_DOXYGEN_END
 int Bar();
 
-// expected-warning@+2  {{unknown command tag name 'encode'; did you mean 'endcode'?}}
-// expected-warning@+1  {{'\endcode' command does not terminate a verbatim text block}}
+// expected-warning@+3  {{unknown command tag name 'encode'; did you mean 'endcode'?}}
+// expected-warning@+2  {{'\endcode' command does not terminate a verbatim text block}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE+1]]:6-[[@LINE+1]]:12}:"endcode"
 /// \encode PR18051
 int PR18051();
-
-// CHECK: fix-it:"{{.*}}":{6:12-6:22}:"a"
-// CHECK: fix-it:"{{.*}}":{10:12-10:15}:"aaa"
-// CHECK: fix-it:"{{.*}}":{14:13-14:23}:"T"
-// CHECK: fix-it:"{{.*}}":{19:13-19:18}:"SomeTy"
-// CHECK: fix-it:"{{.*}}":{26:1-26:1}:"[[ATTRIBUTE]] "
-// CHECK: fix-it:"{{.*}}":{30:1-30:1}:"[[ATTRIBUTE]] "
-// CHECK: fix-it:"{{.*}}":{35:3-35:3}:"[[ATTRIBUTE]] "
-// CHECK: fix-it:"{{.*}}":{39:3-39:3}:"[[ATTRIBUTE]] "
-// CHECK: fix-it:"{{.*}}":{47:3-47:3}:"[[ATTRIBUTE]] "
-// CHECK: fix-it:"{{.*}}":{51:3-51:3}:"[[ATTRIBUTE]] "
-// CHECK: fix-it:"{{.*}}":{76:3-76:3}:"[[ATTRIBUTE]] "
-// CHECK: fix-it:"{{.*}}":{81:3-81:3}:"[[ATTRIBUTE]] "
-// CHECK14: fix-it:"{{.*}}":{87:3-87:3}:"[[ATTRIBUTE]] "
-// CHECK: fix-it:"{{.*}}":{97:1-97:1}:"MY_ATTR_DEPRECATED "
-// CHECK14: fix-it:"{{.*}}":{104:1-104:1}:"ATTRIBUTE_DEPRECATED "
-// CHECK: fix-it:"{{.*}}":{110:6-110:11}:"return"
-// CHECK: fix-it:"{{.*}}":{114:6-114:11}:"foobar"
-// CHECK: fix-it:"{{.*}}":{123:6-123:12}:"endcode"

llvm/unittests/ADT/StringRefTest.cpp

TEST(StringRefTest, Count) {
   StringRef NonOverlappingAbba("abbaabba");
   EXPECT_EQ(2U, NonOverlappingAbba.count("abba"));
   StringRef ComplexAbba("abbabbaxyzabbaxyz");
   EXPECT_EQ(2U, ComplexAbba.count("abba"));
 }
 
 TEST(StringRefTest, EditDistance) {
   StringRef Hello("hello");
+  EXPECT_EQ(2U, Hello.edit_distance("hellooo"));
+  EXPECT_EQ(2U, Hello.edit_distance("hel"));
+  EXPECT_EQ(2U, Hello.edit_distance("Hallo"));
   EXPECT_EQ(2U, Hello.edit_distance("hill"));
 
   StringRef Industry("industry");
   EXPECT_EQ(6U, Industry.edit_distance("interest"));
 
   StringRef Soylent("soylent green is people");
   EXPECT_EQ(19U, Soylent.edit_distance("people soiled our green"));
   EXPECT_EQ(26U, Soylent.edit_distance("people soiled our green",
                                       /* allow replacements = */ false));
   EXPECT_EQ(9U, Soylent.edit_distance("people soiled our green",
                                       /* allow replacements = */ true,