gribozavr2 added a comment.
Thank you for the patch!
================
Comment at: clang/docs/ReleaseNotes.rst:68
+- -Wdocumentation properly handles Doxygen comments with multiple identifiers
in
+ one \param command. The validation whether the name of the identifier is
valid
+ has been improved.
----------------
validation *of*
================
Comment at: clang/docs/ReleaseNotes.rst:360
+- Function ``clang_ParamCommandComment_getNumParams`` has been added to
+ determine the number of parameters to a single Doxygen \param command.p
+- Function ``clang_ParamCommandComment_isVarArgParam`` has been added to
----------------
Need backticks around \param. Also a spurious "p" at the end of the line.
================
Comment at: clang/include/clang-c/Documentation.h:383
CINDEX_LINKAGE
-CXString clang_ParamCommandComment_getParamName(CXComment Comment);
----------------
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.
================
Comment at: clang/include/clang/AST/Comment.h:715
private:
- /// Parameter index in the function declaration.
- unsigned ParamIndex;
+ /// The index of the identifier in the function declaration.
+ MutableArrayRef<unsigned> ParamIndex;
----------------
"Indices of the parameters referenced by this command in the function
declaration."
================
Comment at: clang/include/clang/AST/Comment.h:763
- StringRef getParamName(const FullComment *FC) const;
+ void setParamIndex(MutableArrayRef<unsigned> Idx) {
+ assert(ParamIndex.empty() && "Can't initialize more than once.");
----------------
`setParamIndices`
================
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);
----------------
Users can already call getArgText, do we need a second function that does the
same?
================
Comment at: clang/include/clang/AST/Comment.h:793
+
+ SourceRange getParamRange(unsigned Idx) const LLVM_READONLY {
+ return getArgRange(Idx);
----------------
Ditto, duplicated API.
================
Comment at: clang/include/clang/AST/Comment.h:797
- void setIsVarArgParam() {
- ParamIndex = VarArgParamIndex;
- assert(isParamIndexValid());
+ const Argument *getParam(unsigned Idx) const LLVM_READONLY {
+ assert(Idx < Args.size());
----------------
I'd call it `getArgument` and define it in `BlockCommandComment`.
================
Comment at: clang/include/clang/AST/Comment.h:804
+ assert(Idx < ParamIndex.size());
+ return !isVarArgParam(Idx) && ParamIndex[Idx] != InvalidParamIndex;
}
----------------
Why does vararg have an invalid index? Previously it was considered valid.
================
Comment at: clang/include/clang/AST/CommentSema.h:111
+ void
+ actOnParamCommandParamNameArg(ParamCommandComment *Command,
+ ArrayRef<BlockCommandComment::Argument> Args);
----------------
s/NameArg/NameArgs/
================
Comment at: clang/include/clang/Basic/DiagnosticCommentKinds.td:113
+def warn_doc_param_undocumented : Warning<
+ "parameter '%0' is not documented">,
+ InGroup<Documentation>, DefaultIgnore;
----------------
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.
================
Comment at: clang/lib/AST/Comment.cpp:376
+ case InvalidParamIndex:
+ return getParamName(Idx);
+
----------------
Previously, it was necessary for the index to be valid. Why relax the contract?
(This change also makes things in consistent with, say, TParam).
================
Comment at: clang/lib/AST/CommentParser.cpp:362
+ do {
+ if (Retokenizer.lexIdentifier(Arg)) {
+ ArgumentTokens.push_back(Arg);
----------------
Adding `lexIdentifier()` seems too complicated to me (it is also not *really*
an identifier, because we are allowing "..."). I'd stick to consuming
characters until a comma or a space, that's a lot simpler. If we lexed an
invalid identifier, it won't match any function parameter, and we will get a
warning at that point anyway.
================
Comment at: clang/lib/AST/CommentSema.cpp:742
- // Comment AST nodes that correspond to \c ParamVars for which we have
+ // Comment AST nodes that correspond to \\c ParamVars for which we have
// found a \\param command or NULL if no documentation was found so far.
----------------
Why double-backslash?
================
Comment at: clang/lib/AST/CommentSema.cpp:744
// found a \\param command or NULL if no documentation was found so far.
- SmallVector<ParamCommandComment *, 8> ParamVarDocs;
+ // SmallVector<ParamCommandComment *, 8> ParamVarDocs;
+ SmallVector<const SourceRange *, 8> ParamVarDocs;
----------------
Stale comment (SmallVector)?
================
Comment at: clang/lib/AST/CommentSema.cpp:745
+ // SmallVector<ParamCommandComment *, 8> ParamVarDocs;
+ SmallVector<const SourceRange *, 8> ParamVarDocs;
+ const SourceRange *VarArgDocs = nullptr;
----------------
If `ParamVarDocs` is a vector of SourceRanges, the comment shouldn't say that
it contains AST nodes.
================
Comment at: clang/lib/AST/CommentSema.cpp:758
+ WarnUndocumentedParameters = true;
+ StringRef ParamName = PCC->getParamName(Idx);
----------------
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.
================
Comment at: clang/lib/AST/CommentSema.cpp:852
+ OrphanedParamDecls.erase(OrphanedParamDecls.begin() +
+ CorrectedParamIndex);
+ }
----------------
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.
================
Comment at: clang/lib/AST/CommentSema.cpp:866
+ dyn_cast<FunctionDecl>(ThisDeclInfo->CurrentDecl);
+ assert(FD);
+
----------------
Use cast<> instead.
cast<> = dyn_cast<> + assert
================
Comment at: clang/lib/AST/CommentSema.cpp:1121
+ if (isFunctionOrMethodVariadic() &&
+ Typo.find_first_not_of('.') == StringRef::npos)
+ return ParamCommandComment::VarArgParamIndex;
----------------
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.
================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:1586
+ for (unsigned I = 0; I != E; ++I) {
+ Params.push_back(C->getParamName(FC, I));
+
----------------
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.
================
Comment at: clang/lib/Index/CommentToXML.cpp:745
- if (C->isParamIndexValid()) {
- if (C->isVarArgParam())
- Result << "<IsVarArg />";
- else
- Result << "<Index>" << C->getParamIndex() << "</Index>";
+ Result << "<Parameter>";
+ unsigned E = C->getNumParams();
----------------
Please also update the XML schema in
llvm-project/clang/bindings/xml/comment-xml-schema.rng.
================
Comment at: clang/lib/Index/CommentToXML.cpp:748
+ if (E) {
+ Result << "<Identifiers>";
+ for (unsigned I = 0; I != E; ++I) {
----------------
Why do we need an extra "<Identifiers>" wrapping the list of parameters?
================
Comment at: clang/test/Sema/warn-documentation.cpp:268
+// expected-warning@+1 {{'\param' command does not have a valid identifier}}
+/// \param 9aaa xxx
----------------
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"?
================
Comment at: clang/test/Sema/warn-documentation.cpp:276
+
+// expected-warning@+1 {{'\param' command's identifier is not properly
terminated}}
+/// \param aaa#aaa xxx
----------------
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.
================
Comment at: clang/test/Sema/warn-documentation.cpp:389
/// \param a Aaa.
int test_param21(int a);
----------------
Would be nice to also add a test for '\param a, a Aaa.'
================
Comment at: clang/unittests/AST/CommentParser.cpp:183
+ ParamCommandComment::PassDirection Direction,
+ bool IsDirectionExplicit, ArrayRef<StringRef> ParamName,
+ ParagraphComment *&Paragraph) {
----------------
s/ParamName/ParamNames/
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
