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

[Valentin Clement via Phabricator via llvm-commits]

clementval added a comment.

Just updated the patch.



================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:69
+
+  // Returns the value of the includeHeader field.
+  StringRef getIncludeHeader() const {
----------------
clementval wrote:
> 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. 
I have removed the comments in the wrapper since they do not provide more information than the one in `DirectiveBase.td`. I point to this file for more information.


================
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"); }
+};
----------------
jdenny wrote:
> This conflates `isImplicit` and `isDefault`.
Fixed that. 


================
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 {
----------------
jdenny wrote:
> Should this comment be different than the one on `Clause`?
Yes. I just fixed it. 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84612