<div dir="ltr">Sorry for the incomplete comment -- I pressed "send" too soon. I've edited my comment on Phabricator to add the rest of my reply. (Pointing this out here for people who are reading this in email.)</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 23 Feb 2022 at 15:53, Martin Böhme via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">mboehme added a comment.<br>
<br>
Sorry for the delay -- finally back from leave.<br>
<br>
High-level, I think I'd like to return to my earlier approach <<a href="https://lists.llvm.org/pipermail/cfe-dev/2021-October/thread.html#69087" rel="noreferrer" target="_blank">https://lists.llvm.org/pipermail/cfe-dev/2021-October/thread.html#69087</a>> of adding a new `annotate_type` attribute (see also discussion above).<br>
<br>
In D114235#3244054 <<a href="https://reviews.llvm.org/D114235#3244054" rel="noreferrer" target="_blank">https://reviews.llvm.org/D114235#3244054</a>>, @aaron.ballman wrote:<br>
<br>
> An `AttributedType` for an annotation attribute would require a new type in the type system,<br>
<br>
Not sure I understand what you mean here?<br>
<br>
FWIW, the intent would be to consider types with different `annotate_type` attributes to be the same type (and also the same as the un-attributed type). For example, it would not be possible for two overloads to differ merely by `annotate_type` attributes on their parameters.<br>
<br>
Or are you saying this would require us to add a new subclass of `Type` to the Clang implementation -- e.g. `AnnotatedType`?<br>
<br>
> so there could likely be places that need updating to look "through" the attributed type. But unlike with arbitrary plugins, the number of places is hopefully more reasonable.<br>
<br>
IIUC, this already happens to some extent for other type attributes -- correct? For example, the nullability attributes (`_Nonnull` and the like). However, these can only be applied to pointer types (e.g. `int * _Nonnull ptr`), while an `annotate_type` attribute could also be applied to other types (e.g. `int [[clang::annotate_type("foo")]] i`). Is this an example of the places you're thinking of that would need to be touched?<br>
<br>
>> If the same concerns apply to both of these approaches, do you have any suggestions for alternative ways that I could add annotations to types? Or does this mean that I would have to do the work of making sure that Clang can handle `AttributedType`s in these new places after all?<br>
><br>
> No, I think you basically have to muck about with the type system no matter what. I love the idea of plugin type attributes in theory, but I don't think we're architecturally in a place where we can do that very easily. I suspect a dedicated attribute may be easier, but that's largely a guess. Both approaches strike me as likely to have a long tail of bugs we'll have to fight through.<br>
<br>
What would be your recommendation for the best way to approach this? I.e. after I've implemented an `annotate_type` attribute (I already have a draft patch at <a href="https://reviews.llvm.org/D111548" rel="noreferrer" target="_blank">https://reviews.llvm.org/D111548</a>), how would I best test it for possible issues? I would obviously try to make my tests as comprehensive as possible -- but is there anything else I could do? There obviously isn't any existing code that uses this attribute -- so should I try to locally patch the compiler in some way where it would add the attribute to as many places in the AST as possible, in an attempt to break things?<br>
<br>
In D114235#3244088 <<a href="https://reviews.llvm.org/D114235#3244088" rel="noreferrer" target="_blank">https://reviews.llvm.org/D114235#3244088</a>>, @erichkeane wrote:<br>
<br>
> Right, yeah, so there are a couple of problems with AttributedType. First, it gets lost almost as soon as you get out of SemaType about 90% of the time. Anything that does some level of canonicalization ends up losing it, so the AttributedType information is lost almost immediately. This is why the current ones all store information in the ExtInfo. The worst place for this ends up being in the template instantiator, which immediately canonicalizes/desugars types all over the place.<br>
><br>
> However, making AttributedType 'survive' is actually going to be troublesome as well. A lot of the type-checking compares types using == on their pointer values, so that would be broken if they are an AttributedType.<br>
><br>
> So I think the 'first' thing that needs to happen is to make the entire CFE 'AttributedType' maintaining, AND tolerant. I can't think of a good way to do that reliably (my naive thought would be to come up with some way to temporarily (during development) wrap EVERY type in an AttributedType with a random attribute (so that they are all unique!) and do comparisons that way. Additionally, you'd need SOMETHING to validate that the AttributedTypes are the only one that survives (again, during development) to make sure it 'works right'.<br>
><br>
> Additionally, you'll likely have a lot of work to do in the template engine to make sure that the attributes are inherited correctly through instantiations, specializations, etc.<br>
<br>
Thanks for pointing out those issues!<br>
<br>
I think, if possible, I'd like to avoid having to make `AttributedType<br>
<br>
<br>
Repository:<br>
rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
<a href="https://reviews.llvm.org/D114235/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D114235/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D114235" rel="noreferrer" target="_blank">https://reviews.llvm.org/D114235</a><br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><span><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:16px;font-family:Arial;color:rgb(0,0,0);font-weight:700;vertical-align:baseline;white-space:pre-wrap;background-color:transparent">Martin </span><font color="#000000" face="Arial"><span style="font-size:16px;white-space:pre-wrap"><b>Brænne</b></span></font></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:13.3333px;font-family:Arial;color:rgb(102,102,102);vertical-align:baseline;white-space:pre-wrap;background-color:transparent">Software Engineer</span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:13.3333px;font-family:Arial;color:rgb(102,102,102);vertical-align:baseline;white-space:pre-wrap;background-color:transparent"><a href="mailto:mboehme@google.com" target="_blank">mboehme@google.com</a></span><br></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><br></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span><span style="font-size:10pt;font-family:Arial;color:rgb(102,102,102);background-color:transparent;vertical-align:baseline;white-space:pre-wrap"><span style="border:none;display:inline-block;overflow:hidden;width:130px;height:48px"><img src="https://lh3.googleusercontent.com/0h4r5vSiCkVQWUJnZee4YR8tvgAGLZWeq2v1wdj1Ngn4gmVy0zIUYtLLBoLm6409EeeGWlrfqPU7dkpbkN5Dx4RBO3VRiI9WAPZY3WYjtvPTcmUU4vn6nL6cNMvjFX4_equklVWj" width="130" height="48" style="margin-left: 0px; margin-top: 0px;"></span></span></span></p></span><div><div style="font-size:13px"><span style="font-family:Arial,Verdana,sans-serif"><font color="#666666">Google Germany GmbH</font></span></div><div><div style="font-size:13px;font-family:Arial,Verdana,sans-serif"><font color="#666666">Erika-Mann-Straße 33</font></div><div style="font-size:13px;font-family:Arial,Verdana,sans-serif"><font color="#666666">80363 München<br></font></div><div style="font-size:13px;font-family:Arial,Verdana,sans-serif"><font color="#666666"><br></font></div><div style="font-family:Arial,Verdana,sans-serif"><span><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:10.6667px;font-family:Arial;color:rgb(183,183,183);vertical-align:baseline;white-space:pre-wrap;background-color:transparent">Geschäftsführer: Paul Manicle, Liana Sebastian</span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:10.6667px;font-family:Arial;color:rgb(183,183,183);vertical-align:baseline;white-space:pre-wrap;background-color:transparent">Registergericht und -nummer: Hamburg, HRB 86891</span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:10.6667px;font-family:Arial;color:rgb(183,183,183);vertical-align:baseline;white-space:pre-wrap;background-color:transparent">Sitz der Gesellschaft: Hamburg</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:10.6667px;font-family:Arial;color:rgb(183,183,183);vertical-align:baseline;white-space:pre-wrap;background-color:transparent">Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.</span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:10.6667px;font-family:Arial;color:rgb(183,183,183);vertical-align:baseline;white-space:pre-wrap;background-color:transparent"> </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:10.6667px;font-family:Arial;color:rgb(183,183,183);vertical-align:baseline;white-space:pre-wrap;background-color:transparent">This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks.</span></p></span></div></div></div></div></div></div></div></div></div></div></div></div></div>