[PATCH] D97072: [OpenCL][Docs] Add guidelines for adding new extensions and features

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/docs/OpenCLSupport.rst:196
+If an extension adds the functionality that does not modify standard language
+parsing it should not require clang source code modifications. Most commonly
+such extensions add functionality via libraries (by adding new non-native
----------------
svenvh wrote:
> This might need some rephrasing to take TableGen BIFs into account.  I would consider `clang/lib/Sema/OpenCLBuiltins.td` an integral part of the clang source code.
> 
> Maybe rephrase "clang source code modifications" into "clang parser source code modifications"?
I am trying to avoid using parsing in relation to clang since someone might understand it as clang `Parser` library functionality. I have added a note about this case explicitly

> Note that new functionality in ``OpenCLBuiltins.td`` detailed in :ref:`<opencl_builtins>` belong to this category even if it is part of the clang source code.

Although we do refer to header functionality in the next paragraph anyway where this is referenced but indirectly. I hope it provides enough clarifications for now.


================
Comment at: clang/docs/OpenCLSupport.rst:201
+
+Clang has standard headers where new types and functions are being added,
+for more details refer to
----------------
svenvh wrote:
> Should this mention `clang/lib/Sema/OpenCLBuiltins.td` too?
I think here I don't want to go to too many details of how headers are implemented. I refer to this: https://clang.llvm.org/docs/UsersManual.html#opencl-header that also refers to `OpenCLSupport` page where we explain details on Tablegen header. However, the header topic does require another round of clarifications and I do plan to improve it further iin the near future. 


================
Comment at: clang/docs/OpenCLSupport.rst:221
+triggers in the parsing) should not be added to clang. Moreover, the acceptable
+behavior of pragmas should provide useful functionality to the user.
+
----------------
svenvh wrote:
> Remove "acceptable behavior of".
> 
> How do you define "useful functionality"?
I think this is one of those situations that is hard to define unambiguously, so I am open to suggestions. However, I do want to prevent changes that are reinventing the wheel (i.e. C/C++ already had another way to do the same thing) or functionality that doesn't add any value (i.e. requiring pragma enable to use already added types or functions). This has happened in the past multiple times so I think it is good to be specific that when new functionality is added it should have a reason for it.

I think the other statement above:

> New functionality is accepted as soon as the documentation is detailed to the level sufficient to be implemented.

is similar. It is not very easy to tell what is the detailed level of documentation. FYI it generally aligns with clang overall policy:

https://clang.llvm.org/get_involved.html

> A specification: The specification must be sufficient to understand the design of the feature as well as interpret the meaning of specific examples. The specification should be detailed enough that another compiler vendor could implement the feature.

I am just emphasizing this item here.

Regarding 'usefulness' I would even suggest adding it as a general guideline for clang but I think this is not very common. So for now I want to make sure we have it in our guidelines at least since we have encountered this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97072/new/

https://reviews.llvm.org/D97072