I have "refactored" the X86AsmPrinter into a number of files ready for the MASM and NASM backends to be added.
There is a new namespace llvm::X86 to replace the anomonous namespace as this does not work accross mutiple .h and .cpp files. Other than that everything is pritty straight forward, t may possibly need tweeking though.
Ok, here are my requests. Overall the code looks really good. I would appreciate it if you could make the following changes:
1. At the top of the X86AsmPrinter.cpp file, you note that the code was
refactored. The comments should indicate the current state of the
code, not the history. Please just say that it supports the X/Y/Z
subclasses, not that it was changed to support them. Likewise in any
other similar comments. In the CVS commit message, we will explain the
history of the code.
2. In the .cpp files, please use the 'using namespace llvm;' idiom to
avoid placing the entire C++ file in two sets of namespaces (which
indents everything). Check out other .cpp files for how they are
written as examples.
3. Please #include X86ASmPrinter.h first in X86asmPrinter.cpp (see the
coding standard for justification). This will expose the fact that the
.h file needs some #includes to be self contained.
4. I think it would be a good idea to make the isScale/isMem methods
inline in the header file, even if that means more #includes are
needed in the .h file. These are pretty performance sensitive for the
printers, so it would be good for them to be inlined.
5. Please add #include guards around the headers (#ifndef XX_H/#define
XX_H), like the other llvm headers.
Once you've made these high level changes, please send the code out again and I'll check it once more. Again, the high-level refactoring looks great, and I'm glad you seperated the refactoring from the addition of
It has been built under MS VS2003, but I am not sure how to add it to the makefiles for the Cygwin and Linux platforms, help on this would be appreciated.
There should be no changes required for the Makefiles, unlike those silly VC project files
Afaik there are no specific tests for the X86AsmPrinter. If not it would be good to create some.
Correct. X86AsmPrinter is currently only used for debugging: no targets use it. I suspect your target will be the first real test! This also means that if it is doing something wrong, you can change it!
Also I am wondering about how to go about creating tests for the MASM and NASM backends, hints and help are welcomed.
I think the best way to do this is to start pumping code through it. If you can get the llvmgcc front-end to work in your environment, llvm-test is the place to start.
Thanks for the great enhancement!