Closed Bug 1187486 Opened 5 years ago Closed 5 years ago

clang-plugin's implicit constructor analysis misses templated constructors

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(11 files)

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: 5 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.