Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Public API: ignore methods outside of class #1580

Merged
merged 3 commits into from
Oct 29, 2018
Merged

Public API: ignore methods outside of class #1580

merged 3 commits into from
Oct 29, 2018

Conversation

guwirth
Copy link
Collaborator

@guwirth guwirth commented Oct 27, 2018


This change is Reviewable

@guwirth guwirth added this to the 1.2 milestone Oct 27, 2018
@guwirth guwirth self-assigned this Oct 27, 2018
* add exception: Function definitions with a qualified name outside of a class. Assumption is that they are already documented inside of the class.
* close #1543, #1544
Copy link
Contributor

@ivangalkin ivangalkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 6 files at r1.
Reviewable status: 5 of 6 files reviewed, 7 unresolved discussions (waiting on @guwirth and @ivangalkin)


cxx-squid/src/main/java/org/sonar/cxx/visitors/AbstractCxxPublicApiVisitor.java, line 506 at r1 (raw file):

  private void visitFunctionDefinition(AstNode functionDef) {
    if (isPublicApiMember(functionDef)) {
      

here and below trailing white-spaces


cxx-squid/src/main/java/org/sonar/cxx/visitors/AbstractCxxPublicApiVisitor.java, line 821 at r1 (raw file):

if (node.getType().equals(CxxGrammarImpl.functionDefinition)) {

same as

if (node.is(CxxGrammarImpl.functionDefinition)) {

sonar-c-plugin/src/main/resources/org/sonar/l10n/c/rules/c/UndocumentedApi.html, line 10 at r1 (raw file):
I am not really happy with the wording.

Function definitions with a qualified name outside of a class

What about...

Member functions defined outside of a class/struct.

...? I believe, that "member function" is more precise term, "struct" should be mentioned too, IMHO


sonar-c-plugin/src/main/resources/org/sonar/l10n/c/rules/c/UndocumentedApi.html, line 15 at r1 (raw file):

templates

AbstractCxxPublicApiVisitor has indeed some special logic for detecting of templatized class (struct?) declarations. Not sure if (and how) should they be mentioned in the documentation

 <li>classes/structures (incl. class/struct templates)</li>

...or maybe...

 <li>classes/structures (incl. class/struct templates)</li>
 <li>templates of classes/structures</li>

... but then we would need the same addition about the class members, functions etc.

Or what do you mean by templates?


sonar-c-plugin/src/main/resources/org/sonar/l10n/c/rules/c/UndocumentedApi.html, line 20 at r1 (raw file):

  <li>enumerations</li>
  <li>enumeration values</li>

not sure if "enumeration types" and "enumerators" are more correct terms. See https://en.cppreference.com/w/cpp/language/enum


sonar-c-plugin/src/main/resources/org/sonar/l10n/c/rules/c/UndocumentedApi.html, line 22 at r1 (raw file):

  <li>enumeration values</li>
  <li>typedefs</li>
  <li>alias declaration (<code>using MyAlias = int;</code>)</li>

type alias declarations ?


sonar-c-plugin/src/main/resources/org/sonar/l10n/c/rules/c/UndocumentedApi.html, line 24 at r1 (raw file):

  <li>alias declaration (<code>using MyAlias = int;</code>)</li>
  <li>functions</li>
  <li>variables</li>

global variables?

Copy link
Contributor

@ivangalkin ivangalkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 6 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @guwirth)

@ivangalkin
Copy link
Contributor

@guwirth detection of outside-of-the-class declarations are similar to the one from MethodNameCheck. So the change looks good to me. My comments are very minor. Feel free to ignore.

@guwirth
Copy link
Collaborator Author

guwirth commented Oct 29, 2018

@ivangalkin thx for your review. Tried to fix it. Is the description now better?

Copy link
Contributor

@ivangalkin ivangalkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @guwirth)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants