[cfe-commits] TableGen backend API refactoring review request

Matthieu Monrocq matthieu.monrocq at gmail.com
Tue Jun 5 11:24:03 PDT 2012

On Tue, Jun 5, 2012 at 8:34 AM, Sean Silva <silvas at purdue.edu> wrote:

> The two attached patches change the TableGen backend API (of LLVM and
> clang respectively). They are git diff'd against current ToT. They must
> both be applied "atomically" because the Clang changes depend on LLVM
> header changes (is there a way to avoid this problem without leaving in the
> old code in with a "don't touch, will be removed soon" comment?).
> ### Description of the changes
> These patches change the TableGen backend API as described in <
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-May/049530.html> (I'm
> working on documentation component, but I'm planning on having that be a
> separate patch, submitted after the one that actually changes the API is
> committed). The new backend API is just:
> EmitFoo(RecordKeeper &, raw_ostream & /*, anything else you need */);
> which is simpler and more flexible than the current one. Another benefit
> is that backends can be in their own .cpp file, without needing a pointless
> stub header; additionally, everything inside the .cpp file can be put in an
> anonymous namespace except EmitFoo.
> Although the patches are >2K lines each, it is mostly mechanical
> refactoring churn. Generally, the following changes were made:
> * remove header files whose only use was to declare classes inheriting
> from TableGenBackend. if the class was literally just a stub around the
> FooEmitter::run method, I just made the `run` method become an `EmitFoo`
> function and deleted the stub class. if there was a more substantial class
> that holds state for the emission process, I brought the declaration down
> into the .cpp file and then made EmitFoo just be a stub calling
> FooEmitter(Records).run(OS).
> * centralize the declaration of all of the TableGen backends into
> {llvm,clang}/utils/TableGen/TableGenBackends.h
> * clean out include/llvm/TableGen/TableGenBackend.h. remove class
> TableGenBackend, with accompanying changes to TableGenBackend.cpp.
> EmitSourceFileHeader was preserved, but made a freestanding function with
> name changed to start with a lowercase letter.
> * made appropriate changes to {llvm,clang} TableGen.cpp files to use the
> new API.
> * within the backends, I wrapped stuff in an anonymous namespace as I saw
> opportunities to do so that the new API afforded and I was "in the area".
> I built the "check" and "clang-test" targets with these changes applied
> and they both pass (are there other test targets i should run?).
> ### Specific feedback requests
> I'm in the air about where to declare the backends namespacewise. In the
> LLVM TableGen (which I converted first), I decided to put all the backends
> inside a namespace "tblgen_backends" inside namespace llvm, which turns out
> to be a pain in the ass, since you have to write
> namespace llvm {
> namespace tblgen_backends{
> EmitFoo(RecordKeeper &RK, raw_ostream &OS) {
>   /* emit stuff */
> }
> } // End llvm namespace
> } // End tblgen_backends namespace
> inside the backend's .cpp file. In the clang TableGen, I decided to go
> leaner and just have the backends in namespace "tblgen_backends". I'm
> considering also just having the backends be in the global namespace, since
> it doesn't really matter that much and is more convenient to write.
> Another area that I have questions is whether the filenames should be
> changed, since they are all called FooEmitter.cpp, but sometimes a class
> FooEmitter doesn't exist anymore. My general feeling is that it doesn't
> really matter, so best to just leave them. This is easily changed in a
> future patch if it is deemed desirable.
> These patches are hopefully pretty much ready to go into trunk besides the
> issues I just mentioned. Some general nitpicking would also be appreciated.
> --Sean Silva
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Regarding the namespace issue perhaps that a middle ground approach would
work ?

I believe that:

    namespace llvm { namespace tblgen {

    }} // namespace tblgen, llvm

is not much more typing than just "namespace llvm" and still nicely isolate
the functions so they don't accidentally conflict with other stuff.

On the other hand.. since they are likely to live in entirely separate
binaries anyway, perhaps just "namespace llvm" would be fine (it's either
that or "using namespace::llvm;" as it will use lots of llvm classes and
functions anyway; so I'd just rather nest into the namespace).

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120605/e59d0428/attachment.html>

More information about the cfe-commits mailing list