Clang Plugin should consider values of type AlignedStorage2<T> to inherit attributes from T

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: Nika, Assigned: Nika)

Tracking

unspecified
mozilla42
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

We started discussing this issue in bug 1185044. 

There are two strategies for implementing this, one would be to add a T member to the AlignedStorage2 union, taking advantage of C++11 union like classes. The other would be to add another attribute like MOZ_INHERIT_TYPE_ATTRIBUTES_FROM_TEMPLATE_PARAMS which would cause the type to inherit any type attributes which are present on its template parameters.

The bulk of the discussion on this topic so far can be found in bug 1185044.
This patch includes the work required to make AlignedStorage2's union contain an item of type T on my local computer. This won't build on MSVC 2013, so we can't land this exact patch on master, we'll have to #ifdef out the new code on older compilers.

Comment 2

4 years ago
After reading the discussion in bug 1185044 and the patch, I don't think doing this is worth it.  Should we introduce a MOZ_INHERIT_ANNOTATIONS_FROM_TEMPLATE_ARG attribute instead?
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #2)
> After reading the discussion in bug 1185044 and the patch, I don't think
> doing this is worth it.  Should we introduce a
> MOZ_INHERIT_ANNOTATIONS_FROM_TEMPLATE_ARG attribute instead?

At one point while I was writing the patch for bug 1185044, I had implemented that attribute, it's not super difficult to do, and I may even be able to get back the code I had written before with reflog magic (I threw it out because I thought we were going to do this instead :) ).

I'm fine with implementing that annotation again, it shouldn't take very long to add.

Comment 4

4 years ago
Please do!  Thanks.

Updated

4 years ago
Attachment #8638719 - Flags: review?(ehsan) → review+

Comment 7

4 years ago
Comment on attachment 8638720 [details] [diff] [review]
Use MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS to validate the usage of AlignedStorage2

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

::: mfbt/Attributes.h
@@ +485,5 @@
>  #  define MOZ_MUST_USE __attribute__((annotate("moz_must_use")))
>  #  define MOZ_NEEDS_NO_VTABLE_TYPE __attribute__((annotate("moz_needs_no_vtable_type")))
>  #  define MOZ_NON_MEMMOVABLE __attribute__((annotate("moz_non_memmovable")))
>  #  define MOZ_NEEDS_MEMMOVABLE_TYPE __attribute__((annotate("moz_needs_memmovable_type")))
> +#  define MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS               \

Nit: drop all but one of the whitespaces before \.

@@ +486,5 @@
>  #  define MOZ_NEEDS_NO_VTABLE_TYPE __attribute__((annotate("moz_needs_no_vtable_type")))
>  #  define MOZ_NON_MEMMOVABLE __attribute__((annotate("moz_non_memmovable")))
>  #  define MOZ_NEEDS_MEMMOVABLE_TYPE __attribute__((annotate("moz_needs_memmovable_type")))
> +#  define MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS               \
> +    __attribute__((annotate("moz_inherit_type_annotations_from_template_args")))

Please document this further up this file next to the rest of the documentation!
Attachment #8638720 - Flags: review?(ehsan) → review+
Assignee: nobody → michael
can we get a try run, thanks!
Flags: needinfo?(michael)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c6b30a85cc23
https://hg.mozilla.org/mozilla-central/rev/635fb4ea6b50
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.