[cfe-commits] [PATCH] Foundation for Transform registration infrastructure

[Vane, Edwin]

The Transform class is not meant to be a tag but rather a base class for transforms. I just haven't figured out what that interface will look yet. My idea was, as you suggested, just start converting existing transforms (loop-convert and nullptr) and build things as I go.

Some of the heavy-weighted-ness may come from following some of LLVM's Pass stuff too closely. The pimpl idiom used by TransformRegistry is probably not necessary and I'd be happy simplifying. I used the PassRegistrationListener idea just so the way transforms get their command line arguments recognized is similar to opt. I don't expect transform registration listening to be useful for anything else so I could simplify that away too.

Your other 'rant' points come down to me learning style where the coding standard doc doesn't address a point and so I try to derive common practice from the code. Clearly I don't always find the best examples of code. I too like implementation details at the end and not using acronyms for variable names.

So unless I hear otherwise, I'll go ahead and do these simplifications.

-----Original Message-----
From: Manuel Klimek [mailto:klimek at google.com] 
Sent: Tuesday, December 18, 2012 2:43 PM
To: klimek at google.com; Vane, Edwin
Cc: cfe-commits at cs.uiuc.edu; gribozavr at gmail.com
Subject: Re: [PATCH] Foundation for Transform registration infrastructure


  I'm not sure such a heavy-weight design is needed. I haven't looked at llvm's Pass in detail, but I assume it solves a more generic problem? I especially don't see the need for a tag interface (Transform) which is basically never used.

  I think we can use a much more "Transformation"-centric design here.

  I'd vote for adding a few transformations and then extracting what we see is common, instead of coming up with a design up front where I'm not sure how it might or might not fit our use cases...

  I'm all for adding structure early and stealing good design from other places, but in this specific case my prediction is we'll end up with something much simpler.


================
Comment at: cpp11-migrate/TransformRegistry.h:21
@@ +20,3 @@
+class TransformRegistry {
+  TransformRegistryImpl *Impl;
+
----------------
Adding my usual rant that I strongly prefer the implementation details to be at the bottom of the class, instead of at the top, where it's the first thing a user of the class sees.

================
Comment at: cpp11-migrate/TransformRegistry.h:31
@@ +30,3 @@
+  /// Register a Transform with the registry.
+  void registerTransform(const TransformInfo &TI);
+
----------------
Adding another rant (feel free to ignore) about TLAs. I'd name this Info, and the parameters in the other methods Listener.

================
Comment at: cpp11-migrate/TransformRegistry.cpp:23
@@ +22,3 @@
+
+class TransformRegistryImpl {
+public:
----------------
Why is there a need for this Impl class instead of just instantiating the TransformRegistry?


http://llvm-reviews.chandlerc.com/D221