svenvh 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 ---------------- Anastasia wrote: > 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. This makes it very confusing for the reader, because you're referring to "this category" without defining what a category is. How about removing that note, and appending to the line above instead: If an extension adds functionality that does not modify standard language parsing it should not require //modifying anything other than header files and ``OpenCLBuiltins.td`` as detailed in :ref:`<opencl_builtins>`// ================ Comment at: clang/docs/OpenCLSupport.rst:201 + +Clang has standard headers where new types and functions are being added, +for more details refer to ---------------- Anastasia wrote: > 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. Fair enough; with the addition I'm suggesting above my concern would be addressed anyway. ================ 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 ---------------- mantognini wrote: > svenvh wrote: > > > "of //their// behavior", I think. Yes! ================ 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. + ---------------- Anastasia wrote: > 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. I see, though I'm a bit hesitant to accept the word "useful" without any further explanation. Would adding the following make sense? ... should provide useful functionality //that cannot be achieved by other means// to the user. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97072/new/ https://reviews.llvm.org/D97072 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
