[PATCH] D69930: [OpenMP] Introduce the OpenMPOpt transformation pass

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 3 inline comments as done.
jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:19
+/// OpenMP optimizations pass.
+struct OpenMPOptPass : public PassInfoMixin<OpenMPOptPass> {
+  PreservedAnalyses run(LazyCallGraph::SCC &C, CGSCCAnalysisManager &AM,
----------------
ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > Better to use `class`? Also, maybe worth it to mark it as `final`.
> > > > > > I can do that but I do not understand why this would be better (in a lot of situations, including this one).
> > > > > > After all, class will just cause an additional `public` and that is it (for all but MSVC and only if we declare this struct/class again which we never do). Similarly, I have never seen an attempt to inherit from a new-PM style pass and if so it is unclear why we should forbid it (with `final`).
> > > > > Up to you. But if you going to have `private` or `protected` members later, better to make `class` rather than `struct`.
> > > > > Up to you. But if you going to have private or protected members later, better to make class rather than struct.
> > > > 
> > > > Why? They are the same except the default visibility.
> > > Coding standard
> > The Coding standard states, as a rule of thumb, that the right choice here is a struct:
> > 
> > https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords
> > 
> > 
> > 
> > 
> > But if you going to have private or protected members later, better to make class rather than struct.
The important point is, this does not have a private or protected member now.

There are two things to my argument:
 1) We do not design for what might or might not happen as that regularly results in bad design decisions. Assuming one would later change something, e.g., add a protected member, once could change the struct to a class. 
 2) It is unrealistic to assume a PassInfoMixin object like this one to ever have more members. This is just an interface for the pass manager to interact with the pass, and while there are exceptions, most of these interfaces I've seen only contain a run method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69930