Improve style consistency within clang-plugin

RESOLVED FIXED in Firefox 51

Status

()

Core
Rewriting and Analysis
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mystor, Assigned: Paul Bignier)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8774251 [details] [diff] [review]
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.
Attachment #8774251 - Flags: review?(bpostelnicu)
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 :)
Attachment #8774251 - Flags: review?(bpostelnicu) → review?(michael)
(Reporter)

Comment 3

a year ago
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.
Attachment #8774251 - Flags: review?(michael) → review-
(Reporter)

Updated

a year ago
Priority: -- → P3
(Assignee)

Comment 4

a year ago
Created attachment 8782325 [details] [diff] [review]
0001-clang-plugin-style-consistency.patch
Attachment #8774251 - Attachment is obsolete: true
Attachment #8782325 - Flags: review?(michael)
(Reporter)

Comment 5

a year ago
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
Attachment #8782325 - Flags: review?(michael) → feedback?(ehsan)
(Reporter)

Updated

a year ago
Assignee: nobody → paul.bignier

Comment 6

a year ago
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.
Attachment #8782325 - Flags: feedback?(ehsan) → feedback+
(Assignee)

Comment 7

a year ago
Created attachment 8785172 [details] [diff] [review]
0001-clang-plugin-style-consistency.patch

Thank you both for your comments, here's the updated patch!
Regards,
Paul
Attachment #8782325 - Attachment is obsolete: true
Attachment #8785172 - Flags: review?(michael)
(Assignee)

Comment 8

a year ago
Created attachment 8785174 [details] [diff] [review]
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.
(Reporter)

Comment 9

a year ago
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.
Attachment #8785172 - Flags: review?(michael) → review+
(Assignee)

Comment 10

a year ago
Created attachment 8786322 [details] [diff] [review]
0001-clang-plugin-style-consistency.patch

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

Regards,
Paul
Attachment #8785172 - Attachment is obsolete: true
Attachment #8786322 - Flags: review?(michael)
(Reporter)

Comment 11

a year ago
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
Attachment #8786322 - Flags: review?(michael) → review+
(Assignee)

Comment 12

a year ago
Created attachment 8786350 [details] [diff] [review]
0001-clang-plugin-style-consistency.patch

> This should be called MustOverrides

Done, thanks. Adding the tag.
Paul
Attachment #8786322 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed
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
Also, the patches need rebasing.
Keywords: checkin-needed
Maybe rebase it against m-c mercurial repo.
(Assignee)

Comment 16

a year ago
Created attachment 8786654 [details] [diff] [review]
clang-plugin - style consistency with LLVM

MozReview-Commit-ID: 41uSgifPWRw
Attachment #8786654 - Flags: review?(michael)
(Assignee)

Updated

a year ago
Attachment #8785174 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8786350 - Attachment is obsolete: true
(Reporter)

Comment 17

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

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

lgtm
Attachment #8786654 - Flags: review?(michael) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1aec122ea24a2dc0adc5289749c55938d4d7780
Bug 1287458 - clang-plugin - style consistency with LLVM. r=mystor
I pushed it for Paul to avoid further rebase issues.

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b1aec122ea24
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.