[cfe-dev] how should AnnotateAttr be inherited?

[Aaron Ballman via cfe-dev]

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