[PATCH] D71966: [Wdocumentation][RFC] Improve identifier's of \param

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang-c/Documentation.h:383
 CINDEX_LINKAGE
-CXString clang_ParamCommandComment_getParamName(CXComment Comment);
 
----------------
Mordante wrote:
> gribozavr2 wrote:
> > Please don't modify existing APIs in libclang -- it provides a stable API and ABI, and what has shipped, can't be changed. New functionality has to be exposed as new functions, while old functions should be kept working to the extent possible. It means that the resulting API can be subpar, but oh well, a stable ABI is a contract of libclang.
> I thought I had read this API was allowed to change, but required adding information to the release notes. (I can't find it quickly.)
> I'll undo the changes to the existing functions and add new functions instead.
> I thought I had read this API was allowed to change

It would be interesting to find that doc. As far as I understand, libclang has a strict API & ABI stability rule.

> I'll undo the changes to the existing functions and add new functions instead.

Thanks!



================
Comment at: clang/include/clang/AST/Comment.h:781
+  /// @param Idx The index of the identifier in the \\param command.
+  StringRef getParamName(unsigned Idx) const LLVM_READONLY {
+    return getArgText(Idx);
----------------
Mordante wrote:
> gribozavr2 wrote:
> > Users can already call getArgText, do we need a second function that does the same?
> I thought it made sense to offer these helper functions. `getParamName` seems clearer than `getArgText`. Of course it's also possible to add documentation instead of a helper function. What do you prefer?
> 
> Note: This function is the replacement of `getParamNameAsWritten`, so I can also reinstate its former name.
I prefer using `getArgText`. It works uniformly for all block commands.


================
Comment at: clang/lib/AST/Comment.cpp:376
+  case InvalidParamIndex:
+    return getParamName(Idx);
+
----------------
Mordante wrote:
> gribozavr2 wrote:
> > Previously, it was necessary for the index to be valid. Why relax the contract? (This change also makes things in consistent with, say, TParam).
> IMO it makes it easier on the caller site. If it wants the name of the variable in the current `FullComment` context they can simply call this function. Then they don't need to validate whether the index is valid or not. (I intend to do a similar patch for TParam.)
The issue is that it is unclear what kind of name they get -- the as written name, or the mapped one.


================
Comment at: clang/lib/AST/CommentSema.cpp:852
+      OrphanedParamDecls.erase(OrphanedParamDecls.begin() +
+                               CorrectedParamIndex);
+    }
----------------
Mordante wrote:
> gribozavr2 wrote:
> > I'm not sure about this `erase` call. With this change, once a typo claims an identifier, it is gone no matter how bad the typo was. Even if there's a further, closer, typo, that identifier won't be suggested anymore.
> True, but I thought it was not allowed for fixits to give invalid hints. Using the same correction twice will result in new warnings. What do you think?
> True, but I thought it was not allowed for fixits to give invalid hints.

Fixits are allowed to be incorrect. Any fixit can be incorrect semantically (the user actually intended something different), even if it perfectly fixes the C++ and Doxygen issues.

> Using the same correction twice will result in new warnings.

I don't think users will apply all fixits without subsequent review. In an IDE setting, users pick and choose fixits.

Also, not reusing the same correction twice does not guarantee that the fixes are correct; it only ensures that we won't have a new warning about duplicated parameter names.


================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:1586
+    for (unsigned I = 0; I != E; ++I) {
+      Params.push_back(C->getParamName(FC, I));
+
----------------
Mordante wrote:
> gribozavr2 wrote:
> > Despite what the old code does (I didn't review it when it was submitted), I think it is more useful to dump the parameter name as written, not as found in the function declaration.
> You want the behaviour for all AST dumpers? (I assume yes)
Yes, I'd prefer it everywhere.


================
Comment at: clang/lib/Index/CommentToXML.cpp:748
+  if (E) {
+    Result << "<Identifiers>";
+    for (unsigned I = 0; I != E; ++I) {
----------------
Mordante wrote:
> gribozavr2 wrote:
> > Why do we need an extra "<Identifiers>" wrapping the list of parameters?
> What would you propose instead? No "<Indentifiers>" and just a list of "<identifier>" ?
Yep, "<Parameter>" can directly contain multiple "<Identifier>" tags.


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

https://reviews.llvm.org/D71966