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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

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



================
Comment at: llvm/include/llvm/Frontend/OpenACC/ACC.td:604
   let isDefault = 1;
-}
\ No newline at end of file
+}
----------------
This change doesn't appear to be relevant to this patch and can be committed without review anyway.


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:53
+  // Returns the C++ namespaces that enums and function of this directive
+  // language should be place into.
+  StringRef getCppNamespace() const {
----------------
function -> functions

place -> placed


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:58
+
+  // Returns the prefix to apply to directive enums. Returns empty string if
+  // none.
----------------
To the enums or the enumerators within them?  I'm comparing to your comments in `DirectiveBase.td`.


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:64
+
+  // Returns the prefix to apply to clause enums. Returns empty string if none.
+  StringRef getClausePrefix() const {
----------------
Likewise


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:69
+
+  // Returns the value of the includeHeader field.
+  StringRef getIncludeHeader() const {
----------------
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`.


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:130
+
+  // Returns the list of clauses that are allowed only onced for this directive.
+  std::vector<Record *> getAllowedOnceClauses() const {
----------------
onced -> once


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:156
+
+  // Returns the clamng class name for this clause.
+  StringRef getClangClass() const {
----------------
clanmg -> clang

Is the fact that this is optional important to callers?  Likewise for `getFlangClass`.


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:167
+  // Returns true if this clause is flagged as implicit. False otherwise.
+  bool isDefault() const { return Def->getValueAsBit("isImplicit"); }
+};
----------------
This conflates `isImplicit` and `isDefault`.


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:171
+// Wrapper class that contains clause's information defined in TableGen
+// and provides helper methods for accessing them.
+class VersionedClause {
----------------
Should this comment be different than the one on `Clause`?


================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:360
+      OS << "        case " << DirLang.getClausePrefix()
+         << VerClause.getClause().getFormattedName() << ":\n";
+      OS << "          return " << VerClause.getMinVersion()
----------------
I think the above would be easier to read if `VerClause.getClause().getFormattedName()` were stored in a local variable instead of repeated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84612