[cfe-commits] TableGen backend API refactoring review request

[Sean Silva]

And here are the patches.... oops.

On Fri, Jun 8, 2012 at 10:33 PM, Sean Silva <silvas at purdue.edu> wrote:

> Ok, these patches address all of your points.
>
> 1-llvm.patch is the initial changes to the llvm tree. It changes the
> backend api for llvm/utils/TableGen/* and it makes some very minor
> non-breaking changes to include/llvm/TableGen/TableGenBackend.h and
> lib/TableGen/TableGenBackend.h be make emitSourceFileHeader available as a
> free function (the member function still exists to not break Clang's
> TableGen)
> 2-clang.patch changes Clang's TableGen to the new API
> 3-llvm.patch removes struct TableGenBackend and all remnants of the old
> API.
>
> I built `check` and `clang-test` after applying each one against ToT of
> the respective repos; all green.
>
> --Sean Silva
>
>
> On Wed, Jun 6, 2012 at 1:42 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk>wrote:
>
>>
>> On Jun 4, 2012, at 11:34 PM, 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?).
>>
>> Please try to avoid revlock like that. You can just leave struct
>> TableGenBackend around until it isn't used any longer.
>>
>> Also, split the changes to lib/TableGen and include/llvm/TableGen into a
>> third patch.
>>
>> > 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.
>>
>> There's really no need for that. Backends don't go in libraries. Just
>> stick everything in 'llvm'.
>>
>> > 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.
>>
>> Leave them as is.
>>
>> /jakob
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120608/c5267d10/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1-llvm.patch
Type: application/octet-stream
Size: 85853 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120608/c5267d10/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 2-clang.patch
Type: application/octet-stream
Size: 88407 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120608/c5267d10/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 3-llvm.patch
Type: application/octet-stream
Size: 2863 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120608/c5267d10/attachment-0002.obj>