clang-plugin's implicit constructor analysis misses templated constructors

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

unspecified
mozilla42
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(11 attachments)

9.18 KB, patch
Ehsan
: review-
Details | Diff | Splinter Review
7.90 KB, patch
Details | Diff | Splinter Review
1.76 KB, patch
Details | Diff | Splinter Review
5.45 KB, patch
Details | Diff | Splinter Review
8.84 KB, patch
Details | Diff | Splinter Review
1.14 KB, patch
Details | Diff | Splinter Review
1.10 KB, patch
Details | Diff | Splinter Review
942 bytes, patch
Details | Diff | Splinter Review
1.20 KB, patch
Details | Diff | Splinter Review
1.01 KB, patch
Details | Diff | Splinter Review
2.46 KB, patch
Details | Diff | Splinter Review
If we have a class with a templated single argument constructor, like

  template <typename I>
  nsRefPtr(already_AddRefed<I>& aSmartPtr)
    : mRawPtr(aSmartPtr.take())
    // construct from |already_AddRefed|
  {
  }

the clang-plugin currently doesn't complain about the lack of an `explicit` or `MOZ_IMPLICIT` annotation, in spite of the constructor being an implicit constructor. 

I think that this is because these items are listed in the definition of the struct as FunctionTemplateDecl rather than CXXConstructorDecl, which means that our analysis misses them. We should be able to get around that by also checking for FunctionTemplateDecls where the template declaration is a CXXConstructorDecl.

In addition, I believe that this could be avoided by using Matchers rather than the current strategy of using the RecursiveASTVisitor, as the RecursiveASTVisitor, by default, doesn't enter template instantiations, and if we just directly match CXXConstructorDecls, we will match those produced by template instantiations as well.
We can't land this yet because there are loads of violating code in the codebase right now which need to be fixed first, but this is the change to the plugin to get started.

We should open dependent issues for each of the constructors we discover this way, but I don't have time to do that right now.
Attachment #8638797 - Flags: review?(ehsan)
Comment on attachment 8638797 [details] [diff] [review]
Update the clang plugin to detect templated implicit constructors

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

::: build/clang-plugin/clang-plugin.cpp
@@ +746,5 @@
> +    // We don't want deleted constructors.
> +    !Node.isDeleted();
> +}
> +
> +AST_MATCHER(CXXConstructorDecl, isNotExplicitlyImplicit) {

Please create a positive predicate here (isImplicit) and in the matcher code, use unless(isImplicit()).
Attachment #8638797 - Flags: review?(ehsan) → review+
Comment on attachment 8638797 [details] [diff] [review]
Update the clang plugin to detect templated implicit constructors

Actually, as you can see from the change you had to make to JSCompartment, this analysis is not 100% correct, since it only needs to look at declarations in the body of the class, not definitions of constructors.
Attachment #8638797 - Flags: review+ → review-
This patch will help fix the issue I mentioned above.
I fixed up the patch to address my own comments and landed it for you.
(In reply to :Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #15)
> I fixed up the patch to address my own comments and landed it for you.

Thanks :)
Depends on: 1189465
https://hg.mozilla.org/mozilla-central/rev/d88da1f3c511
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.