This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Fix code style of method comments
ClosedPublic

Authored by thopre on May 2 2019, 8:49 AM.

Details

Summary

Fix various issues in code style of method comments:

  1. Move all heading comments to all non-static methods near their declaration in the FileCheck.h header file.
  2. Harmonize the action verb in doxygen comments for methods to always be in third person
  3. Use \returns instead of free text "return" and "returns".
  4. Document a couple more parameters while at it.

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.May 2 2019, 8:49 AM
probinson accepted this revision.May 2 2019, 9:38 AM

Thanks! Some small things, and LGTM once you've fixed those.

llvm/include/llvm/Support/FileCheck.h
257 ↗(On Diff #197795)

I thought \p has to name the parameter? so "whether \p C is..."

271 ↗(On Diff #197795)

What does "the Pattern" mean? There's no member or parameter named Pattern.

276 ↗(On Diff #197795)

Parameter Req is not mentioned.

403 ↗(On Diff #197795)

there are no

434 ↗(On Diff #197795)

I could wish the parameters were described in detail, but that's beyond what you're doing in this patch.

446 ↗(On Diff #197795)

No doc for Diags (but again won't insist for this patch).

This revision is now accepted and ready to land.May 2 2019, 9:38 AM
thopre updated this revision to Diff 197931.May 3 2019, 2:12 AM
thopre marked 6 inline comments as done.
  • Document missing parameter beside SM in ReadCheckFile and CheckInput
  • Add parameter name to description of isValidVarNameStart
  • Rephrase comment for ParsePattern and document Req parameter
thopre added a comment.EditedMay 3 2019, 2:13 AM

Thanks! Some small things, and LGTM once you've fixed those.

Since I've rephrased 2 comments and I'm not a native english speaker I'll let you (@probinson) review those changes. Only once you've acked them will I commit this patch. Thanks for the quick review!

thopre edited the summary of this revision. (Show Details)May 3 2019, 2:19 AM
thopre requested review of this revision.May 8 2019, 9:32 AM

Marking this patch as needing review to double check whether comment update in last version is intelligible and grammatically correct english

Sorry, I should have been more thorough about this previously: My understanding is that if a function description mentions any parameters with \p, then it needs to mention all of them.
For some of the one-liner descriptions, if you prefer to just remove the \p that's okay with me. You've been suffering a lot through this process and I don't want to create more work than we really need to!

llvm/include/llvm/Support/FileCheck.h
271 ↗(On Diff #197931)

"initializes"

286 ↗(On Diff #197931)

"\returns" describes the function return value, not the effect on other parameters. So:
\returns the position that ...
If there is a match, updates \p MatchLen with the size of the matched string.

318 ↗(On Diff #197931)

Returns -> \returns

Description is missing parameter SM.

402 ↗(On Diff #197931)

If you cite any parameters with \p you need to cite all of them.

404 ↗(On Diff #197931)

If you cite any parameters with \p you need to cite all of them.

406 ↗(On Diff #197931)

If you cite any parameters with \p you need to cite all of them.

438 ↗(On Diff #197931)

Need to mention \p SM

451 ↗(On Diff #197931)

Needs to cite \p SM

thopre updated this revision to Diff 198701.May 8 2019, 11:11 AM
thopre marked 8 inline comments as done.
  • Document parameters not mentioned in \p when others in the same function are mentioned
  • Only use \returns for the return value
probinson accepted this revision.May 8 2019, 11:34 AM

One or two last nits and LGTM.

llvm/include/llvm/Support/FileCheck.h
189 ↗(On Diff #198701)

Depending on where this comes in the sequence of other patches, this might want to be "[#]VAR=VAL" (or not, if that syntax will be introduced after this patch, in which case ignore this comment).

439 ↗(On Diff #198701)

record -> records

This revision is now accepted and ready to land.May 8 2019, 11:34 AM
thopre updated this revision to Diff 198717.May 8 2019, 1:35 PM
thopre marked 2 inline comments as done.

Use 3rd person for all verbs in ReadCheckFile comment

This revision was automatically updated to reflect the committed changes.