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

[Alexander Lanin via Phabricator via llvm-commits]

AlexanderLanin marked 6 inline comments as done.
AlexanderLanin added a comment.

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



================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:91
+/// \param ZZZZZZ Does not match at all.
+int test2to2_oneAlmost(int argc, int argv);
+
----------------
Mordante wrote:
> Here there's one undocumented parameter left and one `\param` why not match them?
Because at the time this is executed there are technically two parameters unmatched. One has a fix-it suggestion.


================
Comment at: clang/test/Sema/warn-documentation-fixits.cpp:149
+  */
+void run(const char *const arg[], int argc);
+
----------------
Mordante wrote:
> 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.
Love the idea, will look into it!


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

https://reviews.llvm.org/D74877