[PATCH] [WIP] Large refactoring of AST/APValue

[Gautier DI FOLCO]

Hi doug.gregor,

Hello,

DISCLAIMERS : Firstly, I'm far to be talented at speaking english (more generally I have some troubles with languages, it is hard for me to translate all the shadings of my thoughts), so please, excuse me if I made some errors or if you think that I'm aggressive or whatever (any corrections is welcome, of course). Secondly, I don't question your ability to produce "good" code (it is a really subjective critera) or to take wise design decisions. Finally, I'm a french student, I code in C++ for 10 years, but as student, I had the opportunity to see a large set of subjects, so I haven't take time to walk into the deepest parts of C++ (which doesn't mean that I'm a complete C++ beginner).

As I said in a previous patch proposal, I want to contribute to clang, I want to begin by doing dirty work (doing things that have to be done, but things that no one want to do or have the time to do) which will help me to see the big picture.

I think I know relatively well good OOP design principles (putting behavior in the right places, SOLID principles, Demeter law and all other buzzwords). I perform well in picking legacy code and refactoring it in code that is closer to these principles.

So, I walk into the source tree to find a switch statement (which is generally the smell that some behaviors are not at the right place). I found one in lib/AST/APValue.cpp, I tought I was lucky because I have already worked on an XML/DTD AST representation. So I began to refactor it and, one week later I'm still working on how to integrate cleanly my work with the rest of the codebase.

If you try to apply my patch, it won't compile, it's not finished, it's not clean nor well formatted and I have some design wonderings (for example by merging *Data and *Value or by making the *Value childs of APValue).

My point of view it that we can easily deal with complex design, but complicated code is hard to live with. This module is clearly complicated (high-levels abstractions which rely on low-level ones, multiples casts, switches to determine which kind of instances is this object), surely due to performances considerations.

I have two objectives: keep clang fast (or make it faster) and make it faster to compile by making it easier to read (so easier to maintain, to correct, to evolve it and less bug-prone).

I think that these two objectives are not opposed and they are even closely related. Performances are run-time are only due to the performances of very few pieces of the program, I don't see any reasons to deal with complicated code if performances are not perceptibles. 

So I decided to create a class by APValue Kind (because inheritance is made for that and looking up into a vtable is not necessarly slower than doing N tests and N function calls). I have then get back all the code where there was a switch on the kind and put them into specific methods.

Now I'm working on the integration with lib/AST/ExprConstant.cpp, but it is a big part and before ending that and cleaning my code and making some tests, I want to have or point of view.

Am I in the right way? Have my patch a chance to be committed if I continue? Is this approach good for you?

I could easily understand if you disagree, but if you can take time to explain me why, I will be grateful at you, I'm trying to learn everyday (and the hard way is often the best way to learn).

Thanks for your time and thanks by advance for your answer.

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

Files:
  include/clang/AST/APValue.h
  lib/AST/APValue.cpp
  lib/AST/ASTDiagnostic.cpp
  lib/AST/Decl.cpp
  lib/AST/ExprConstant.cpp
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D3091.1.patch
Type: text/x-patch
Size: 69171 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140316/15651568/attachment.bin>