[PATCH] D112577: [clang][OpenMP] Initial parsing/sema for 'align' clause

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:362-364
+  /// Sets the location of '('.
+  void setLParenLoc(SourceLocation Loc) { LParenLoc = Loc; }
+
----------------
ABataev wrote:
> Also worth to make it private
I agree, I think these constructors should have their access reduced to at least `protected`.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:369
+  /// Returns alignment
+  Expr *getAlignment() const { return cast_or_null<Expr>(Alignment); }
+
----------------
I have a preference for improved const-correctness with this sort of thing, where I'd rather see an overloaded pair of functions:
```
const Expr *getAlignment() const { return cast_or_null<Expr>(Alignment); }
Expr *getAlignment() { return cast_or_null<Expr>(Alignment); }
```




================
Comment at: clang/lib/AST/OpenMPClause.cpp:1615
+  OS << "align(";
+  Node->getAlignment()->printPretty(OS, nullptr, Policy, 0);
+  OS << ")";
----------------
`getAlignment()` can return nullptr and it looks like the vast majority of the calls to it are unprotected like this. Would it make sense to ensure that `OMPAlignClause` can never have a nonnull alignment expression?


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

https://reviews.llvm.org/D112577