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

David Pagan via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Oct 29 13:19:45 PDT 2021


ddpagan added inline comments.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:362-364
+  /// Sets the location of '('.
+  void setLParenLoc(SourceLocation Loc) { LParenLoc = Loc; }
+
----------------
aaron.ballman wrote:
> ABataev wrote:
> > Also worth to make it private
> I agree, I think these constructors should have their access reduced to at least `protected`.
Agreed. I made them private, then created a function to build the clause rather than reference the constructor directly. This is similar to what we do for some other clauses. 


================
Comment at: clang/include/clang/AST/OpenMPClause.h:369
+  /// Returns alignment
+  Expr *getAlignment() const { return cast_or_null<Expr>(Alignment); }
+
----------------
aaron.ballman wrote:
> 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); }
> ```
> 
> 
Agreed. Unfortunately, OpenMP code isn't set up well for this. A few clauses are, but not this one. So, trying to make this change led to an ugly rat's nest of other changes outside the scope of this fix. I'd like to see new clauses follow this pattern, though.


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1615
+  OS << "align(";
+  Node->getAlignment()->printPretty(OS, nullptr, Policy, 0);
+  OS << ")";
----------------
aaron.ballman wrote:
> `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?
This particular code is only called when a legitimate align clause has been used, so we are guaranteed a non-nullptr. I looked over other references to getAlignment (most from other classes) and it didn't appear that any of them were unsafe.


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

https://reviews.llvm.org/D112577



More information about the cfe-commits mailing list