[PATCH] D48162: [GSoC] Schedule tree performance.

[Siddharth via Phabricator via llvm-commits]

bollu requested changes to this revision.
bollu added a comment.
This revision now requires changes to proceed.

I'm going to request changes so this does not show up on the feed. I added two more comments regarding constructors :)



================
Comment at: include/polly/ScheduleOptimizer.h:41
+
+  Reference() {}
+  ~Reference() {}
----------------
Does it make sense to initialize a `Reference` without a `Name`, `Domain`, or `AccessType`? I feel it makes sense to give it a constructor that initializes it, rather than creating an empty object and then initializing "by hand" as done in `classifyReference`


================
Comment at: include/polly/ScheduleOptimizer.h:45
+
+struct Skeleton {
+  std::vector<Reference> References;
----------------
bollu wrote:
> bollu wrote:
> > I am not sure I like the use of `struct` here. Usually, it is use to signal a POD (poor old data type) with no complex constructor. In this case, since it owns an `std::vector` (or may own a `SmallVector`), I would prefer it being tagged `class`.
> > 
> > You could still make everything `public` if that's what you wanted by making it a `struct`.
> Also, why `Skeleton`? Is this some name in the paper?
> 
> Please add a line of description above why this struct is called `Skeleton` and what it tracks.
Similarly, can one have a `Skeleton` without a `name`? It may make sense to add a constructor and a method to `add` and `get` references, but not _remove_ them (IIRC, there is no removal of references, correct?)

In general, having access control would be nice!


https://reviews.llvm.org/D48162