[PATCH] D84612: [openmp][openacc] Add wrapper for records in DirectiveEmitter

[Valentin Clement via Phabricator via llvm-commits]

clementval marked an inline comment as done.
clementval added a comment.

In D84612#2179007 <https://reviews.llvm.org/D84612#2179007>, @jdenny wrote:

> Are you following an existing TableGen emitter as a precedent for this kind of thin wrapper design?

I'm following the way it is done with MLIR table gen usage. You can find a bunch of them in `mlir/include/mlir/TableGen`.

Thanks for your comments. I'll update the patch today.



================
Comment at: llvm/include/llvm/Frontend/OpenACC/ACC.td:604
   let isDefault = 1;
-}
\ No newline at end of file
+}
----------------
jdenny wrote:
> This change doesn't appear to be relevant to this patch and can be committed without review anyway.
Right. Look like it was edited by me editor and made its way in the review ... I'll remove it from this patch. 


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:69
+
+  // Returns the value of the includeHeader field.
+  StringRef getIncludeHeader() const {
----------------
jdenny wrote:
> This documentation effectively defers to documentation in `DirectiveBase.td`.  Other getter documentation here duplicates parts of the field documentation in `DirectiveBase.td` instead.
> 
> I'm not sure which is better, but the documentation should be consistent.  Duplication is obviously bad for maintenance, but does it improve readability here?  An alternative would be to just write one comment that encloses all getters and that defers to `DirectiveBase.td`.
Agreed. I'll update that. 


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:156
+
+  // Returns the clamng class name for this clause.
+  StringRef getClangClass() const {
----------------
jdenny wrote:
> clanmg -> clang
> 
> Is the fact that this is optional important to callers?  Likewise for `getFlangClass`.
Yeah as it might be null. Will update the comment. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84612