Page MenuHomePhabricator

[AST] Add TableGen for StmtDataCollectors
ClosedPublic

Authored by johannes on Sep 1 2017, 9:22 AM.

Details

Summary

This adds an option "-gen-clang-data-collectors" to the Clang TableGen
that is used to generate StmtDataCollectors.inc.

Diff Detail

Repository
rL LLVM

Event Timeline

johannes created this revision.Sep 1 2017, 9:22 AM
Herald added a subscriber: mgorny. · View Herald Transcript
arphaman edited edge metadata.Sep 1 2017, 9:58 AM

TableGen is great for structured data, but we shouldn't use it just to embed code in it. What was the issue you've had with the build when the inc file was in include?

@arphaman I suggested tablegen in D36664 because I remembered we had some CMake sanity check about not having an *.inc in our include dir: https://github.com/llvm-mirror/clang/blob/master/CMakeLists.txt#L266 Not sure if it actually fires for our case, but it looks like it does.

Also it would be nicer to maintain once we have a second attribute for an optional single-TU code as tablegen could just fall back from SingleTUCode to the normal Code attribute. And we would have an easy solution for the CrossTU/SingleTU inc files as tablegen could just create both without us having to manually copy code around or do preprocessor hacks.

I guess this review will make more sense once we have both CrossTU and SingleTU code ready.

@arphaman I suggested tablegen in D36664 because I remembered we had some CMake sanity check about not having an *.inc in our include dir: https://github.com/llvm-mirror/clang/blob/master/CMakeLists.txt#L266 Not sure if it actually fires for our case, but it looks like it does.

I see, thanks.

Also it would be nicer to maintain once we have a second attribute for an optional single-TU code as tablegen could just fall back from SingleTUCode to the normal Code attribute. And we would have an easy solution for the CrossTU/SingleTU inc files as tablegen could just create both without us having to manually copy code around or do preprocessor hacks.

If you think there's a need for TableGen in the future then I see no problem with this approach. I was just reluctant to accept it only for code.

I guess this review will make more sense once we have both CrossTU and SingleTU code ready.

@arphaman I suggested tablegen in D36664 because I remembered we had some CMake sanity check about not having an *.inc in our include dir: https://github.com/llvm-mirror/clang/blob/master/CMakeLists.txt#L266 Not sure if it actually fires for our case, but it looks like it does.

Yes, that was the issue why it didn't work under include/. I think the sanity check could be removed, since in-source builds are forbidden anyway.
Or maybe it is still useful by telling us that .inc is probably an generated file (most, but not all of them are generated, it seems), so another solution would be to just change the file extension. Actually, .def might be the right extension.

Shall we move to using TableGen only when we need it, and rename it to .def for now?
A preprocessor-only solution for the two modes could look like this:

user:

#define DERIVED MyStmtVisitor
#define SINGLE_TU
#include "clang/AST/StmtDataCollectors.def"

StmtDataCollectors.def:

#define DEF_VISIT(CLASS, CODE)\
    template <class = void> void Visit##CLASS(const CLASS *S) {\
      CODE;\
      ConstStmtVisitor<##DERIVED>::Visit##CLASS(S)\
    }

#ifdef SINGLE_TU
#define DEF_ADD_DATA(CLASS, COMMON, SINGLE, CROSS) DEF_VISIT(CLASS, COMMON; SINGLE)
#else
#define DEF_ADD_DATA(CLASS, COMMON, SINGLE, CROSS) DEF_VISIT(CLASS, COMMON, CROSS)
#endif

DEF_ADD_DATA(Stmt, .., .., ..)
...

However, I think TableGen has an advantage when we can extract some structure instead of just storing the code. Basically, a list of primitive and one of pointer members for each class could simplify things.

arphaman accepted this revision.Sep 4 2017, 12:44 AM

defs could work, but I think that we should stick with TableGen. As you've said, we'd be able to introduce some sort of structure instead of just code in the future which will be better (Ideally we'd try to do it now, but given the fact that the GSoC is done the current approach is acceptable). I would also like to see this analyzed and tested better in the future to ensure a complete coverage of AST, and I think the use of TableGen can potentially help with that.

LGTM with one inline comment below:

utils/TableGen/TableGen.cpp
151 ↗(On Diff #113547)

Looks like an 80 column violation, please reformat this line.

@teemperor ok for you? did phabricator make you a blocking reviewer because of the affected code, or did I do that somehow?

teemperor accepted this revision.Sep 6 2017, 6:09 AM

@johannes The blocking reviewer is because it touches clone detection code :) Fine with me, I have some comments on things but nothing that affects this review. LGTM!

This revision is now accepted and ready to land.Sep 6 2017, 6:09 AM
This revision was automatically updated to reflect the committed changes.