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

[Alexander Lanin via Phabricator via llvm-commits]

AlexanderLanin planned changes to this revision.
AlexanderLanin marked 9 inline comments as done.
AlexanderLanin added inline comments.


================
Comment at: clang/lib/AST/CommentSema.cpp:22
+
+#define DEBUG_TYPE "commentsema"
 
----------------
Mordante wrote:
> Is the debug output really that interesting after the code has been committed?
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?


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


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


================
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,
----------------
Mordante wrote:
> I would suggest to rename the function. The current name doesn't match what the function does. For example `findUndocumentedParams`.
Thanks, that should have been obvious from the comment 'Returns all ParmVarDecl without a comment'.


================
Comment at: clang/lib/AST/CommentSema.cpp:873
+          // parameter! Remove old and new correction.
+          LLVM_DEBUG(llvm::dbgs() << "Duplicate suggestion to correct @"
+                                  << ParamName << " to "
----------------
Mordante wrote:
> 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?
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?


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


================
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]]:
----------------
Mordante wrote:
> why test for ZZZZZZ ?
Copy paste error. There is a ZZZZZZ before and after this test case.


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


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

https://reviews.llvm.org/D74877