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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

Some minor typos and requests for clarifications, looks like reasonable guidelines other than that.



================
Comment at: clang/docs/OpenCLSupport.rst:175
+
+If extensions modifies the standard parsing they need to be added to
+the clang frontend source code. This also means that the associate macro
----------------
Probably easier to keep this section singular (i.e. talk about a adding a single extension), instead of switching between extension and extensions all the time.


================
Comment at: clang/docs/OpenCLSupport.rst:176
+If extensions modifies the standard parsing they need to be added to
+the clang frontend source code. This also means that the associate macro
+indicating the presence of the extensions should be added to clang.
----------------
associated


================
Comment at: clang/docs/OpenCLSupport.rst:177
+the clang frontend source code. This also means that the associate macro
+indicating the presence of the extensions should be added to clang.
+
----------------
Again, keep it singular.


================
Comment at: clang/docs/OpenCLSupport.rst:179
+
+The default flow for adding the new extensions into the frontend is to
+modify `OpenCLExtensions.def
----------------



================
Comment at: clang/docs/OpenCLSupport.rst:195
+
+If an extension adds the functionality that does not modify standard language
+parsing it should not require clang source code modifications. Most commonly
----------------



================
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
----------------
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"?


================
Comment at: clang/docs/OpenCLSupport.rst:201
+
+Clang has standard headers where new types and functions are being added,
+for more details refer to
----------------
Should this mention `clang/lib/Sema/OpenCLBuiltins.td` too?


================
Comment at: clang/docs/OpenCLSupport.rst:212
+
+Clang provides mechanism to add the standard extension pragma
+``OPENCL EXTENSION`` by setting a dedicated flag in the extension list entry of
----------------



================
Comment at: clang/docs/OpenCLSupport.rst:216
+standard extension pragmas as it is not specified (for the standards up to and
+including version 3.0) with the sufficient level of details and, therefore,
+there is no default functionality provided by clang.
----------------



================
Comment at: clang/docs/OpenCLSupport.rst:219
+
+Pragmas without detailed information of its behavior (explanation of changes it
+triggers in the parsing) should not be added to clang. Moreover, the acceptable
----------------



================
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.
+
----------------
Remove "acceptable behavior of".

How do you define "useful functionality"?


================
Comment at: clang/docs/OpenCLSupport.rst:226
+the use of types or functions. This functionality is not guaranteed to remain in
+the future releases, however, any future changes should not affect backward
+compatibility.
----------------



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

https://reviews.llvm.org/D97072