1201309 - Make MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS work with MOZ_NON_MEMMOVABLE

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

The ananlysis for MOZ_NON_MEMMOVABLE uses different code from the other storage-class-ish attributes, so it doesn't currently respect MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS.  This should be fixed, so that we can detect memmove problems with an intervening use of mozilla::Maybe or other sum-type-like containers that use AlignedStorage2.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

One approach is to just loosely copypaste the template argument check into the memmovable checker; this gets us the… not maximally helpful error message seen in bug 1198979 comment #5.  Instead, if I reuse the reason-tracking we already have (and remove ~100 lines of code in the process), the error looks like this:

> ../../dist/include/nsTArray.h:662:34: error: Cannot instantiate 'nsTArray_CopyChooser<mozilla::layers::Edit>' with non-memmovable template argument 'mozilla::layers::Edit'
> struct MOZ_NEEDS_MEMMOVABLE_TYPE nsTArray_CopyChooser
>                                  ^
> ../../dist/include/nsTArray.h:793:42: note: instantiation of 'nsTArray_CopyChooser<mozilla::layers::Edit>' requested here
>   : public nsTArray_base<Alloc, typename nsTArray_CopyChooser<E>::Type>
>                                          ^
> /home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:10222:11: note: 'mozilla::layers::Edit' is a non-memmove()able type because member 'mValue' is a non-memmove()able type 'mozilla::layers::Edit::Value'
>     Value mValue;
>           ^
> /home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:9506:56: note: 'mozilla::layers::Edit::Value' is a non-memmove()able type because member 'VOpSetLayerAttributes' is a non-memmove()able type 'mozilla::AlignedStorage2<OpSetLayerAttributes>'
>         mozilla::AlignedStorage2<OpSetLayerAttributes> VOpSetLayerAttributes;
>                                                        ^
> ../../dist/include/mozilla/Alignment.h:126:56: note: 'mozilla::AlignedStorage2<OpSetLayerAttributes>' is a non-memmove()able type because it has a template argument non-memmove()able type 'mozilla::layers::OpSetLayerAttributes'
> struct MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS AlignedStorage2
>                                                        ^
> /home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:5490:21: note: 'mozilla::layers::OpSetLayerAttributes' is a non-memmove()able type because member 'attrs_' is a non-memmove()able type 'LayerAttributes' (aka 'mozilla::layers::LayerAttributes')
>     LayerAttributes attrs_;
>                     ^
> /home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:5276:27: note: 'LayerAttributes' (aka 'mozilla::layers::LayerAttributes') is a non-memmove()able type because member 'common_' is a non-memmove()able type 'CommonLayerAttributes' (aka 'mozilla::layers::CommonLayerAttributes')
>     CommonLayerAttributes common_;
>                           ^
> /home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:4247:12: note: 'CommonLayerAttributes' (aka 'mozilla::layers::CommonLayerAttributes') is a non-memmove()able type because member 'contentDescription_' is a non-memmove()able type 'string' (aka 'basic_string<char>')
>     string contentDescription_;
>            ^

(This is also using work-in-progress for bug 1201314 and bug 1201329.)

Comment 2

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

I agree that you should re-use the CustomTypeAnnotation logic for this. The memmovable checker was just missed in the migration to the CustomTypeAnnotation logic :).

If you have any questions about the plugin, just let me know!

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: bug1201309-template-nonmove-hg0.diff

Ehsan: sorry about asking for review when your bugzilla userinfo says not to ask for reviews, but I can't find this on the modules wiki page and revision history suggests you're the person to ask; feel free to adjust the attachment flags as appropriate.

Comment 4

[(no longer active)]

Comment on attachment 8656959 [details] [diff] [review]
bug1201309-template-nonmove-hg0.diff

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

Very nice, thanks for the patch!

Comment 5

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

Comment on attachment 8656959 [details] [diff] [review]
bug1201309-template-nonmove-hg0.diff

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

LGTM too!

Thanks :)

::: build/clang-plugin/clang-plugin.cpp
@@ +747,5 @@
>        Diag.Report(Decl->getLocation(), TemplID) << Pretty << T << Reason.Type;
>        break;
>      }
>      default:
> +      // FIXME: is it possible to point out the original annotation?

Would you file a follow-up bug about this? I originally left this annotation out because I didn't want to clutter up the error output, but I've found myself wanting a reference to where the annotation is located a few times while I've been working on other plugins, so I think we should get this.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: bug1201309-template-nonmove-hg1.diff

Trivial comment change to cite the followup bug.  Thanks for catching that; I don't like landing FIXMEs without attached bug numbers, but I forgot about that one.  And carrying over r+; thanks for the review.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27221624668e

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27221624668e