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

Wed Oct 21 11:28:22 PDT 2020

Mordante added a comment.

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

================
Comment at: clang/lib/AST/CommentSema.cpp:22
+
+#define DEBUG_TYPE "commentsema"

----------------
AlexanderLanin wrote:
> 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?
Yes please. When needed again for debugging it's trivial to add the wanted debug statements.

================
Comment at: clang/lib/AST/CommentSema.cpp:873
+          // parameter! Remove old and new correction.
+          LLVM_DEBUG(llvm::dbgs() << "Duplicate suggestion to correct @"
+                                  << ParamName << " to "
----------------
AlexanderLanin wrote:
> 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?
Yes if it's not trivial a separate patch would be preferable.

================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:81
+/// \param ArgV Almost matches.
+int test2to2_bothAlmost(int argc, int argv);
+
----------------
AlexanderLanin wrote:
> 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.
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.

================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:149
+  */
+void run(const char *const arg[], int argc);
+
----------------
AlexanderLanin wrote:
> AlexanderLanin wrote:
> > 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.
> 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?!
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.

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

https://reviews.llvm.org/D74877