[cfe-dev] Minor patch controversial

Fri Mar 3 18:17:09 PST 2017

On Fri, Mar 3, 2017 at 4:06 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On 3 March 2017 at 09:32, Aaron Ballman via cfe-dev <cfe-dev at lists.llvm.org>
> wrote:
>>
>> On Fri, Mar 3, 2017 at 1:04 AM, Marcwell Helpdesk via cfe-dev
>> <cfe-dev at lists.llvm.org> wrote:
>> > Hi everyone!
>> >
>> > A few weeks back we sent a minor patch to the cfe-commit list that we
>> > thought would be a non-issue once it had passed the review phase, but that
>> > was not the case, instead we were told that the patch was too controversial
>> > so we ask for a general opinion on the matter.
>> >
>> > The current implementation of the annotate attribute supports annotation
>> > of declarations using the GNU syntax, with its data forwarded into AST and
>> > further down to IR. The supported set of declarations includes classes,
>> > variables, fields, functions and methods. The patch extends this to
>> > annotation of statements and the use of C++11 syntax but with the lack of
>> > support in LLVM IR the data is only forwarded onto AST.
>> >
>> > Overall, this is a very minor change to what already is present and
>> > being used, but the small addition could benefit many plugin and tool
>> > developers readily today. The concept of annotations are very generic and
>> > makes it possible to insert, for example, control directives or tags
>> > directly into the source code that a plugin or tool may read and utilize.
>> > Please note that the intention of the patch is to make a small adjustment,
>> > not to alter the meaning of annotations into something it is not.
>> >
>> > Here is an example usage:
>> >
>> >     File: xtool.hpp
>> >     #define XTOOL_A “xtool:directiveA”
>> >     #define XTOOL_B “xtool:directiveB"
>> >
>> >     File: sample.cpp
>> >     #include <xlibrary.hpp>
>> >
>> >     [[clang::annotate(XTOOL_A]]
>> >     int main(int argc, char* argv[])
>> >     {
>> >         // This is what the patch adds, possibility to annotate a
>> > statement
>> >         [[clang::annotate(XTOOL_B)]]
>> >         while( N )
>> >         {  . . .  }
>> >         return 0;
>> >     }
>> >
>> > The controversy of the patch appears when comparing annotations with
>> > pluggable attributes and suggesting that the two technologies competes for
>> > the same goal. No, they do not, for the simple reason that an annotation is
>> > an annotation, nothing more, nothing less, and should stay that way. It
>> > should not have the same streamlined functionality of PA, such as proper
>> > namespacing, argument checking etc. And let us be very clear on one thing;
>> > having pluggable attributes would be a great addition to Clang and we’re not
>> > trying dissuade anyone from implementing it by promoting annotations
>> > instead. The patch does not in any way make annotations move closer to PA
>> > than it was before.
>> >
>> > One of the complaints in the "annotations vs PA” discussion is how the
>> > functionality/information are exposed to both end-users and attribute
>> > authors, being error-prone to use. This is a complaint that actually
>> > underlines that annotations are not competing with pluggable attributes, it
>> > may be fit for some solutions but for others a more strict and controlled
>> > environment, like pluggable attributes, may be required. Diversity and
>> > different levels of support is what makes Clang superior to the competition.
>> > There is no reason to stop using an existing functionality until an
>> > alternative is available and to our humble understanding, implementing an
>> > architecture that supports PA up front and in both AST and IR probably needs
>> > a few iteration to set things straight, pushing the availability date into
>> > the distant future.
>> >
>> > We ask for this small patch to be committed since it makes a minor
>> > enhancement (annotated statements) to an existing functionality that
>> > benefits the community of plugin and tool writers today by providing a
>> > generic and consistent way of communicating control directives/tags directly
>> > from the source code to itself, such as source code transformers,
>> > generators, collectors and similar.
>>
>> For reference, the review thread starts at:
>>
>> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170130/184405.html
>>
>> (Unfortunately, it appears to not be threaded properly, so you may
>> have to search for the replies to follow along with the discussion.)
>>
>> While this is a simple adjustment to what the annotate attribute
>> appertains to, I do not want to extend the attribute in that
>> direction. As stated during the review, the annotate attribute is
>> already problematic in that there is no indication of what the
>> specific annotations should appertain to, no ability to supply
>> arguments, no way to prevent different tools from name collisions,
>> etc. Basically, the annotate attribute was a very quick way to do what
>> it needs to do back when we didn't have a good path towards pluggable
>> attributes. Now that we do have the good path forward, we should not
>> expand the capabilities of a deficient, competing solution simply
>> because it's the path of least resistance -- that does not provide our
>> users or tool authors with a good experience. I do not find the
>> argument that extending the annotate attribute does not compete with
>> pluggable attributes because it is more error-prone to be a compelling
>> rationale.
>
>
> Thanks, having an eye on the big picture, particularly regarding our
> vendor-specific extensions, is important.
>
> How far along is the work on pluggable attributes -- is there anything that
> would help make progress on that?

I believe we now have all the components needed to implement them, but
do not have concrete progress that surfaces the feature.
Unfortunately, I do not have time to work on it in the short term, but
I believe the implementation should be relatively straight-forward and
I am happy to provide direction and reviews to anyone interested in
working on it.

~Aaron

>
>> As mentioned in the review, I am fine with adding the C++ spelling for
>> the annotate attribute as that is certainly non-controversial and is
>> an incremental improvement.
>>
>> ~Aaron
>>
>> >
>> > Cheers,
>> > Chris
>> >
>> > _______________________________________________
>> > cfe-dev mailing list
>> > cfe-dev at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>