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

[Mark de Wever via Phabricator via llvm-commits]

Mordante added a comment.

>> 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.



================
Comment at: clang/lib/AST/CommentSema.cpp:735
+                                        bool IsFunctionOrMethodVariadic) {
+  for (unsigned i = 0, e = ParamVars.size(); i != e; ++i) {
+    const IdentifierInfo *II = ParamVars[i]->getIdentifier();
----------------
Please capitalize the `i` and `e`.


================
Comment at: clang/lib/AST/CommentSema.cpp:783
+
+  for (auto PCC : PCCs) {
+    if (PCC->isVarArgParam()) {
----------------
Since the `auto` type is a pointer, please use a `*` to make it clear.


================
Comment at: clang/lib/AST/CommentSema.cpp:789
+             "PCCs and ParmVars vectors do not match");
+      if (auto PrevPCC =
+              ParmVarDeclToParamCommandComment[PCC->getParamIndex()]) {
----------------
Please don't use `auto` unless the type is clear.


================
Comment at: clang/lib/AST/CommentSema.cpp:794
+        Diags.Report(PrevPCC->getLocation(), diag::note_doc_param_previous)
+            << PrevPCC->getParamNameRange();
+      }
----------------
Is it intended that the assignment `ParmVarDeclToParamCommandComment[PCC->getParamIndex()] = PCC;` is executed? If so please add some comment.


================
Comment at: clang/lib/AST/CommentSema.cpp:804
 
-  // 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();
+  for (unsigned i = 0, e = ParmVars.size(); i != e; ++i)
+    if (!ParmVarDeclToParamCommandComment[i])
----------------
Please capitalize the `i` and `e`.


================
Comment at: clang/lib/AST/CommentSema.cpp:816
+
+  for (auto PCC : ParamComments)
+    if (!PCC->isParamIndexValid())
----------------
Since the `auto` type is a pointer, please use a `*` to make it clear.


================
Comment at: clang/lib/AST/CommentSema.cpp:846
+  else {
+    for (unsigned i = 0, e = UnresolvedParamComments.size(); i != e; ++i) {
+
----------------
Please capitalize the `i` and `e`.


================
Comment at: clang/lib/AST/CommentSema.cpp:866
+
+  for (unsigned i = 0, e = UnresolvedParamComments.size(); i != e; ++i) {
+    if (ParmVarForComment[i]) {
----------------
Please capitalize the `i` and `e`.


================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:74
+
+// This test matches bug report at https://bugs.llvm.org/show_bug.cgi?id=43808.
+// Note: first parameter is the same as above, bug gets auto fixed here since
----------------
Why use a different test case than in the bug report? I really would like to see this patch fixes that case.


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

https://reviews.llvm.org/D74877