[cfe-dev] how should AnnotateAttr be inherited?

[Aaron Ballman via cfe-dev]

On Sun, May 6, 2018 at 1:26 PM, Jacob Bandes-Storch
<jacob at bandes-storch.net> wrote:
> Thank you, isInherited() is what I was looking for :)
>
> On the topic of attributes in general, I found it curious that clang
> completely drops unrecognized attributes and they aren't made available in
> any Attr (as far as I could tell), and thus can't be checked by custom
> plugins, etc. Do you know if there's a specific reason for this? Why not
> keep the original spelling around in something like "UnrecognizedAttr"?

Personally, I would like to see something along these lines, where the
UnknownAttr (or whatever we call it) wraps as much information as
possible about the parsed attribute, but at a bare minimum keeps the
source location information about where the attribute arguments start
and end and what syntax was used to specify the attribute.

This might be reasonable for declaration attributes, but I'm less
clear on how it would look for attributes which appertain to a type
(whether we build up an AttributedType for it or not) or statements
(do we build an AttributedStmt for it or not), but we'd want to do
something to handle those as well.

~Aaron

>
> Jacob
>
> On Sun, May 6, 2018 at 7:08 AM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> On Sat, May 5, 2018 at 11:26 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > +Aaron
>> >
>> > Tangentially related:
>> >
>> > It strikes me our current inheritance behavior for AnnotateAttr (ie,
>> > inherit
>> > unless already present) makes no sense, as we have no idea what the
>> > attribute means or how it should be inherited from one declaration to
>> > another. If one declaration has the attribute and a redeclaration has
>> > the
>> > attribute with a different string, under what circumstances should we
>> > drop
>> > the old attribute? It seems to me that the only sensible options are
>> > "inherit all attributes even if one is already present on the new
>> > declaration" (eg, set InheritEvenIfAlreadyPresent) and "never inherit an
>> > AnnotateAttr" (eg, make it not be an InheritableAttr).
>> >
>> > I have no particular preference between those two; either option seems
>> > like
>> > it creates some extra hassle for some users of the attribute, and less
>> > hassle for others, but the status quo seems wrong. Aaron, what do you
>> > think?
>>
>> I agree that the status quo is wrong, but I don't have a strong sense
>> for whether we should always inherit or never inherit. I kind of feel
>> that always inheriting but only if there's a difference in attributes
>> would be in the spirit of the annotate attribute's "this is totally
>> generic and can mean anything" design. However, I don't think there's
>> a "clearly correct" behavior here, which also suggests that perhaps it
>> should be an error (or at least a warning) to try to inherit annotate
>> attributes with different arguments.
>>
>> ~Aaron
>>
>> >
>> >
>> > On 5 May 2018 at 20:18, Richard Smith <richard at metafoo.co.uk> wrote:
>> >>
>> >> On 5 May 2018 at 13:33, Jacob Bandes-Storch via cfe-dev
>> >> <cfe-dev at lists.llvm.org> wrote:
>> >>>
>> >>> I am using an AST matcher and would like to prevent an annotation
>> >>> attribute from being placed on an out-of-line constructor definition.
>> >>>
>> >>>     class Foo {
>> >>>         explicit Foo(int);  // ok
>> >>>         [[clang::annotate("implicit")]] Foo(int);  // ok
>> >>>     };
>> >>>
>> >>>     explicit Foo::Foo() {}  // disallowed
>> >>>     [[clang::annotate("implicit")]] Foo::Foo(int) {}  // should be
>> >>> disallowed
>> >>>
>> >>>
>> >>> However it seems that when I match the out-of-line decl,
>> >>> `decl->specific_attrs<AnnotateAttr>()` contains the attribute even if
>> >>> it was
>> >>> placed on the original in-class declaration and not on the out-of-line
>> >>> definition.
>> >>>
>> >>> How can I determine whether an Attr* was written on the out-of-line
>> >>> decl?
>> >>
>> >>
>> >> You can look over the AnnotateAttr(s) on the out-of-line declaration
>> >> and
>> >> check !Attr::isInherited() to see whether they were written on that
>> >> declaration.
>> >>
>> >>>
>> >>> I tried checking (!(attr->getLocation() < decl->getLocStart()) &&
>> >>> !(decl->getLocEnd() < attr->getLocation())), however it seems the two
>> >>> locations are not comparable. When I look at their getRawEncoding()s,
>> >>> the
>> >>> attribute has values in ranges like 2147587494-2147587527 while the
>> >>> decl's
>> >>> values are more reasonable, such as 3049-3079.
>> >>
>> >>
>> >> The numerical values of source locations (and their comparison order
>> >> under
>> >> <) have no strong relation to the semantics of the program and should
>> >> be
>> >> thought of as arbitrary; we give a total order under operator< to
>> >> facilitate
>> >> use of SourceLocations in std::map and the like. (You can use
>> >> SourceManager::isBeforeInTranslationUnit if you want to compare source
>> >> locations in order of appearance, in cases where that makes sense.)
>> >> Currently, the high bit of source locations indicates that they
>> >> represent a
>> >> location within a macro expansion (presumably in your actual testcase
>> >> the
>> >> attribute was coming from a macro expansion), but it would be unwise to
>> >> rely
>> >> on that and you should use the high-level semantically-aware
>> >> functionality
>> >> in SourceManager instead.
>> >>
>> >>>
>> >>> I couldn't find any documentation about attribute source locations
>> >>> being
>> >>> negative.
>> >>
>> >>
>> >> The documentation comment for the SourceLocation class describes the
>> >> encoding we currently use:
>> >>
>> >> /// Technically, a source location is simply an offset into the
>> >> manager's
>> >> view
>> >> /// of the input source, which is all input buffers (including macro
>> >> /// expansions) concatenated in an effectively arbitrary order. The
>> >> manager
>> >> /// actually maintains two blocks of input buffers. One, starting at
>> >> offset
>> >> /// 0 and growing upwards, contains all buffers from this module. The
>> >> other,
>> >> /// starting at the highest possible offset and growing downwards,
>> >> contains
>> >> /// buffers of loaded modules.
>> >> ///
>> >> /// In addition, one bit of SourceLocation is used for quick access to
>> >> the
>> >> /// information whether the location is in a file or a macro expansion.
>> >
>> >
>
>