Mordante marked 14 inline comments as done.
Mordante added a comment.
Thanks for the review!
I only answered to the larger issues to get a better feeling of the direction
the patch should go. Once that's clear I'll make a new patch and also address
the smaller issues.
================
Comment at: clang/include/clang-c/Documentation.h:383
CINDEX_LINKAGE
-CXString clang_ParamCommandComment_getParamName(CXComment Comment);
----------------
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.
================
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);
----------------
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.
================
Comment at: clang/include/clang/AST/Comment.h:804
+ assert(Idx < ParamIndex.size());
+ return !isVarArgParam(Idx) && ParamIndex[Idx] != InvalidParamIndex;
}
----------------
gribozavr2 wrote:
> Why does vararg have an invalid index? Previously it was considered valid.
Before a lot of code first tested whether the index was valid and then
depending on the vararg state do a different codepath. I thought it was very
confusing that if the function returned `true` it was no guarantee it was safe
to access `DeclInfo::ParamVars[Idx]`.
================
Comment at: clang/include/clang/Basic/DiagnosticCommentKinds.td:113
+def warn_doc_param_undocumented : Warning<
+ "parameter '%0' is not documented">,
+ InGroup<Documentation>, DefaultIgnore;
----------------
gribozavr2 wrote:
> This warning should be in a separate group. `-Wdocumentation` contains
> generally useful, non-controversial stuff (that would have been an error if
> documentation comments were a part of the language). Warning about every
> undocumented parameter is only useful for certain style guides.
>
> (Digression: And even then, I personally question the value of a style rule
> that requires every parameter (or every public API, or every file, or every
> whatever else) to be documented. Unless there's a tech writer on the team, in
> response to a such rule engineers are highly likely just to write useless
> comments like "\param frobnicator The Frobnicator necessary for this call."
> Engineers are already doing the best they can when writing comments. The
> choice we have is not between "no comments for some parameters" and "good
> comments for every parameter because the warning reminded you", it is between
> "no comments for some parameters" and "placeholder stuff that people will
> write to make the warning go away".)
>
> I'd also suggest to implement this functionality in a separate patch.
I'll move it to its own patch.
================
Comment at: clang/lib/AST/Comment.cpp:376
+ case InvalidParamIndex:
+ return getParamName(Idx);
+
----------------
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.)
================
Comment at: clang/lib/AST/CommentSema.cpp:758
+ WarnUndocumentedParameters = true;
+ StringRef ParamName = PCC->getParamName(Idx);
----------------
gribozavr2 wrote:
> Here's one reason why I find the fallback in `getParamName` to be
> questionable -- with the fallback, the function does different things
> depending on the phase of processing. Sometimes it returns the original name
> as written (before the index is set), and sometimes it returns the name from
> the function declaration (which can be different, as far as I understand).
> I'd much prefer to call `getParamNameAsWritten` here.
The difference is now in the `FullComment` argument instead of the name. But it
seems that is not clear.
================
Comment at: clang/lib/AST/CommentSema.cpp:852
+ OrphanedParamDecls.erase(OrphanedParamDecls.begin() +
+ CorrectedParamIndex);
+ }
----------------
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?
================
Comment at: clang/lib/AST/CommentSema.cpp:866
+ dyn_cast<FunctionDecl>(ThisDeclInfo->CurrentDecl);
+ assert(FD);
+
----------------
gribozavr2 wrote:
> Use cast<> instead.
>
> cast<> = dyn_cast<> + assert
Thanks for the hint!
================
Comment at: clang/lib/AST/CommentSema.cpp:1121
+ if (isFunctionOrMethodVariadic() &&
+ Typo.find_first_not_of('.') == StringRef::npos)
+ return ParamCommandComment::VarArgParamIndex;
----------------
gribozavr2 wrote:
> I'm not sure I like the heuristic.
>
> ```
> // \param foo.bar Something.
> void foo(int foo_bar, ...);
> ```
>
> Here we would suggest to correct `foo.bar` to `...`.
>
> Instead, I'd suggest to throw a "..." identifier into the typo corrector if
> the function is variadic, and let the edit distance decide.
This heuristic works at the moment. The `lexIdentifier` will not accept
`foo.bar`. However since you requested to remove the `lexIdentifier` your
approach makes more sense.
================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:1586
+ for (unsigned I = 0; I != E; ++I) {
+ Params.push_back(C->getParamName(FC, I));
+
----------------
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)
================
Comment at: clang/lib/Index/CommentToXML.cpp:748
+ if (E) {
+ Result << "<Identifiers>";
+ for (unsigned I = 0; I != E; ++I) {
----------------
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>" ?
================
Comment at: clang/test/Sema/warn-documentation.cpp:268
+// expected-warning@+1 {{'\param' command does not have a valid identifier}}
+/// \param 9aaa xxx
----------------
gribozavr2 wrote:
> Seems redundant to say that the identifier should be valid. Also it is not
> just any identifier, we can be more specific. WDYT about "a parameter name
> should be specified after the '\param' command"?
Agreed.
================
Comment at: clang/test/Sema/warn-documentation.cpp:276
+
+// expected-warning@+1 {{'\param' command's identifier is not properly
terminated}}
+/// \param aaa#aaa xxx
----------------
gribozavr2 wrote:
> I don't think it makes sense to distinguish "not a valid identifier" and "not
> properly terminated". From the user's perspective, the issue is the same.
I'm not entirely sure. However after I remove the `lexIdentifier` `9aaa` will
become a valid identifier. So I'll remove this warning.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71966/new/
https://reviews.llvm.org/D71966
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
