[NFC][SYCL] Use visitors to print kernel name type #2660
Conversation
This is the part of integration header generator refactoring to make it use clang visitors and simplify the code.
|
Hi, I have a note for reviewers, there is a couple of questions over the code. The first one is about printing of "class" keyword before names of classes which don't belong to any namespace. I saved the old behavior, but it seems that if we don't print it resulting integration header is still valid. |
I think the 'flush' function is a good idea then. The destructor seems in retrospect like an odd place for that. As far as the 'class' keyword, I'd prefer it not be there if not necessary. However, this patch is sufficiently large of an improvement over the old code that I'd think an investigation into it could happen at a later date if you so wished. |
Thanks! Then I'll add 'flush' function and I think I'll leave 'class' keyword investigation to the next patches to keep this one smaller and NFC. |
@erichkeane Can you elaborate why? We have a couple of places in SemaSYCL where we use destructors for different tasks. Why is it weird to use it to emit output? Anyway, other than comments by Erich, the patch looks good to me. Thanks! |
Actually, in reprospect now that I see more of what it is doing, I don't think the destructor is an inappropriate place for it, unless further patches show a reason why it wouldn't be required. But having the destructor do the 'commit to file' part is perfectly fine. I think it actually works well there. In general, my problem is with the emitWithoutAnonNamespaces function in itself, it seems that should be a part of the printing policy or something. |
I was able to stop using |
even better! |
f6ec961
This is the part of integration header generator refactoring to make
it use clang visitors and simplify the code.