1287458 - Improve style consistency within clang-plugin

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

Unlike the rest of central, the clang plugin is intended to be formatted with llvm-style, but it has many inconsistencies as to how exactly it is formatted. We should probably add some comments explaining the specific style used in this file, and update all variable names etc. in the file to match the llvm style.

One of the large inconsistencies (which is found within llvm too) is as to whether methods and variables should be lowercase-c camelCase or uppercase-C CamelCase. We should decide on one option and make sure to change all variables in the file to match that style.

Comment 1

[Paul Bignier]

Attachment: 0001-Clang-plugin-format.patch

Made an attempt at using lower-C camelCase on variables + functions but it cannot be carried out fully since the file uses external variables such as 'Cache' that should be renamed elsewhere. So I just went for the clang-format patch.

Comment 2

[Andi [:andi]]

Comment on attachment 8774251 [details] [diff] [review]
0001-Clang-plugin-format.patch

I'm not a reviewer for this code, so i will forward it to mystor :)

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8774251 [details] [diff] [review]
0001-Clang-plugin-format.patch

Review of attachment 8774251 [details] [diff] [review]:
-----------------------------------------------------------------

The main goal which I had with this patch was to move our use of LLVM-style closer to the actual specification in the LLVM Coding Standards Guide: http://llvm.org/docs/CodingStandards.html

For the most part this patch seems to make automated changes with clang-format which are not necessarily desirable (such as the reformatting of parameters to astMatcher.addMatcher).

Ideally a patch for this bug would correct the main style inconsistency within this module: The variable naming, and would add a comment to the top with the high level summary of the style, and a link to the above document.

Here's a summary of the variable naming rules in LLVM: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

* Type names (including classes, structs, enums, typedefs, etc) should be nouns and start with an upper-case letter (e.g. TextFileReader).

* Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats).

* Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).

* Enum declarations (e.g. enum Foo {...}) are types, so they should follow the naming conventions for types. A common use for enums is as a discriminator for a union, or an indicator of a subclass. When an enum is used for something like this, it should have a Kind suffix (e.g. ValueKind).

::: build/clang-plugin/clang-plugin.cpp
@@ +535,4 @@
>  };
>  
>  /// A cached data of whether classes are refcounted or not.
> +typedef DenseMap<const CXXRecordDecl *, std::pair<const Decl *, bool> >

I'm not sure we want the spaces between the >> - All of the compilers we support support them being adjacent.

@@ +991,5 @@
> +                                     hasUnaryOperand(allOf(
> +                                         hasType(builtinType()),
> +                                         anyOf(hasDescendant(declRefExpr()),
> +                                               declRefExpr())))))
> +                                             .bind("node"))))).bind("call"),

The formatting of the arguments to astMatcher.addMatcher calls are usually very explicit. I would prefer to not reformat these.

@@ +1116,4 @@
>  // probably will be used at some point in the future, in order to produce better
>  // error messages.
>  typedef DenseMap<const MaterializeTemporaryExpr *, const Decl *>
> +AutomaticTemporaryMap;

This feels less readable than the original.

Comment 4

[Paul Bignier]

Attachment: 0001-clang-plugin-style-consistency.patch

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8782325 [details] [diff] [review]
0001-clang-plugin-style-consistency.patch

Review of attachment 8782325 [details] [diff] [review]:
-----------------------------------------------------------------

I have a few comments. I'd like to see an updated version with these comments fixed.

f?-ing ehsan to make sure that he's Okay with the style changes, because this does make changes to a lot of lines of code, and might mess with history a bit / he might want a slightly different style in our code.

::: build/clang-plugin/clang-plugin.cpp
@@ +172,5 @@
> +  ExplicitImplicitChecker EIChecker;
> +  NoAutoTypeChecker NATChecker;
> +  NoExplicitMoveConstructorChecker NEMCChecker;
> +  RefCountedCopyConstructorChecker RCCCChecker;
> +  AssertAssignmentChecker AAChecker;

I don't like these names very much. Maybe try names like `NoAddRefReleaseOnReturn` instead of `NARRORChecker`?

@@ +441,5 @@
>        }
>      }
>    }
>  
> +  bool visitCXXRecordDecl(CXXRecordDecl *D) {

I'm pretty sure that the visit methods need to start with a capital V, as they are called from the base class RecursiveASTVisitor<T>. I think they will compile if you typo the name, but not work IIRC.

@@ +573,5 @@
>    // Base class: anyone with AddRef/Release is obviously a refcounted class.
>    if (classHasAddRefRelease(D))
>      return true;
>  
> +  // Look through all Base cases to figure out if the parent is a refcounted

unnecessary capitalization

Comment 6

[(no longer active)]

Comment on attachment 8782325 [details] [diff] [review]
0001-clang-plugin-style-consistency.patch

Review of attachment 8782325 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks fairly good, except for the comments below.

For whitespace formatting, I suggest running clang-format and create a separate patch with *only* whitespace changes and nothing else.  Since there is already a build/clang-plugin/.clang-format file present, you should be able to cd to build/clang-plugin and run clang-format there.  Please do not make manual adjustments to clang-format's output, since those will just get overwritten when the next person runs clang-format.

::: build/clang-plugin/clang-plugin.cpp
@@ +44,5 @@
>  
>  // Check if the given expression contains an assignment expression.
>  // This can either take the form of a Binary Operator or a
>  // Overloaded Operator Call.
> +bool hasSideEffectAssignment(const Expr *Expression) {

FWIW LLVM sometimes uses E for an Expr variable, D for a Decl variable and so on.  I don't particularly care one way or the other.

@@ +172,5 @@
> +  ExplicitImplicitChecker EIChecker;
> +  NoAutoTypeChecker NATChecker;
> +  NoExplicitMoveConstructorChecker NEMCChecker;
> +  RefCountedCopyConstructorChecker RCCCChecker;
> +  AssertAssignmentChecker AAChecker;

I second Michael's comment.  Please don't make the code less readable by choosing variable names with acronyms inside them.  Making the names just CamelCase should be the only thing that's needed here.

@@ +441,5 @@
>        }
>      }
>    }
>  
> +  bool visitCXXRecordDecl(CXXRecordDecl *D) {

Yes.

Comment 7

[Paul Bignier]

Attachment: 0001-clang-plugin-style-consistency.patch

Thank you both for your comments, here's the updated patch!
Regards,
Paul

Comment 8

[Paul Bignier]

Attachment: 0001-Clang-plugin-format.patch

And here's the clang-format patch again.
It was done on master branch but I can provide the same patch on top of the camelCase one if it gets merged.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8785172 [details] [diff] [review]
0001-clang-plugin-style-consistency.patch

Review of attachment 8785172 [details] [diff] [review]:
-----------------------------------------------------------------

If this builds with the clang plugin enabled now (and my nits fixed), I think it's ready to land :). Thanks for your help on this!

Run clang-format on the CamelCase-d version of the code and post that patch too. They can land together.

::: build/clang-plugin/clang-plugin.cpp
@@ +412,2 @@
>  
> +  virtual void handleTranslationUnit(ASTContext &Ctx) {

I'm pretty confident that this method also needs to be named HandleTranslationUnit. While you're here, could you also add an `override` annotation to this method? I believe that it overrides HandleTranslationUnit in ASTConsumer.

@@ +1427,2 @@
>  
> +  // Check every superclass for whether it has a Base with a refcnt member, and

Don't need to change the capitalization of Base in the comment.

Comment 10

[Paul Bignier]

Attachment: 0001-clang-plugin-style-consistency.patch

All fixed, thanks for the review, I'll tag checkin-needed if you r+ again.

Regards,
Paul

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8786322 [details] [diff] [review]
0001-clang-plugin-style-consistency.patch

Review of attachment 8786322 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nit below

::: build/clang-plugin/clang-plugin.cpp
@@ +454,5 @@
>  
>      // Look through all of our immediate bases to find methods that need to be
>      // overridden
>      typedef std::vector<CXXMethodDecl *> OverridesVector;
> +    OverridesVector Must_overrides;

This should be called MustOverrides

Comment 12

[Paul Bignier]

Attachment: 0001-clang-plugin-style-consistency.patch

> This should be called MustOverrides

Done, thanks. Adding the tag.
Paul

Comment 13

[Ryan VanderMeulen [:RyanVM]]

In the future, please try to use commit messages that explain what the patch is doing.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Also, the patches need rebasing.

Comment 15

[Andi [:andi]]

Maybe rebase it against m-c mercurial repo.

Comment 16

[Paul Bignier]

Attachment: clang-plugin - style consistency with LLVM

MozReview-Commit-ID: 41uSgifPWRw

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8786654 [details] [diff] [review]
clang-plugin - style consistency with LLVM

Review of attachment 8786654 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 18

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1aec122ea24a2dc0adc5289749c55938d4d7780
Bug 1287458 - clang-plugin - style consistency with LLVM. r=mystor

Comment 19

[Sylvestre Ledru [:Sylvestre]]

I pushed it for Paul to avoid further rebase issues.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b1aec122ea24