[cfe-dev] Minor patch controversial

[Richard Smith via cfe-dev]

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?

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170303/8caf753b/attachment.html>