This revision was automatically updated to reflect the committed changes.
Closed by commit rL324808: [Templight] Template Instantiation Observer
(authored by xazax, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D5767?vs=133614&id=1337
xazax.hun accepted this revision.
xazax.hun added a comment.
Thanks, this looks good to me! I will try this out soon and commit after that.
https://reviews.llvm.org/D5767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/
sabel83 updated this revision to Diff 133614.
https://reviews.llvm.org/D5767
Files:
include/clang/Driver/CC1Options.td
include/clang/Frontend/FrontendActions.h
include/clang/Frontend/FrontendOptions.h
include/clang/FrontendTool/Utils.h
include/clang/Sema/Sema.h
include/clang/Sema/Temp
Szelethus added a comment.
In https://reviews.llvm.org/D5767#1000347, @xazax.hun wrote:
> In https://reviews.llvm.org/D5767#999143, @sabel83 wrote:
>
> > 2. What do you mean by regression tests? We have run the clang-test target
> > successfully on the patched code (which has the hook). Note tha
xazax.hun added inline comments.
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:646
}
+}
+
Use either `LLVM_FALLTHROUGH;` here or break to avoid compiler warnings.
https://reviews.llvm.org/D5767
___
cf
xazax.hun added inline comments.
Comment at: lib/FrontendTool/ExecuteCompilerInvocation.cpp:261
+
+} //namespace
Nit: this should be `// namespace clang`
https://reviews.llvm.org/D5767
___
cfe-commits mailing list
Szelethus added inline comments.
Comment at: lib/Sema/Sema.cpp:40
#include "clang/Sema/TemplateDeduction.h"
+#include "clang/Sema/TemplateInstCallback.h"
#include "llvm/ADT/DenseMap.h"
xazax.hun wrote:
> Do you need to add this include?
Yes, in `Sema.h` the cla
sabel83 updated this revision to Diff 133400.
sabel83 marked an inline comment as done.
https://reviews.llvm.org/D5767
Files:
include/clang/Driver/CC1Options.td
include/clang/Frontend/FrontendActions.h
include/clang/Frontend/FrontendOptions.h
include/clang/FrontendTool/Utils.h
include/c
xazax.hun added a comment.
In https://reviews.llvm.org/D5767#999143, @sabel83 wrote:
> 2. What do you mean by regression tests? We have run the clang-test target
> successfully on the patched code (which has the hook). Note that the hook
> this pull request provides is implemented as a ProgramA
sabel83 marked an inline comment as done.
sabel83 added inline comments.
Comment at: include/clang/Driver/CC1Options.td:537
+def templight_dump : Flag<["-"], "templight-dump">,
+ HelpText<"Dump templight information to stdout">;
def ast_dump_lookups : Flag<["-"], "ast-dump-look
sabel83 added a comment.
This pull requests consists of two parts:
a) a hook, which is called during template instantiation events
b) a callback, which dumps the details of the events out to standard output
Tools like Templight (https://github.com/mikael-s-persson/templight) rely on
the hook.
xazax.hun added a comment.
Overall looks good. Was this tested on large software? I would also be grateful
if you could run the regression tests with templight always being enabled to
see if they uncover any assertions/crashes.
Comment at: include/clang/Driver/CC1Options.td:5
sabel83 updated this revision to Diff 132778.
https://reviews.llvm.org/D5767
Files:
include/clang/Driver/CC1Options.td
include/clang/Frontend/FrontendActions.h
include/clang/Frontend/FrontendOptions.h
include/clang/FrontendTool/Utils.h
include/clang/Sema/Sema.h
include/clang/Sema/Temp
sabel83 added a comment.
I have no commit access, please commit the patch for me.
https://reviews.llvm.org/D5767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
sabel83 marked 2 inline comments as done.
sabel83 added a comment.
I have extended the context as suggested.
https://reviews.llvm.org/D5767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co
sabel83 updated this revision to Diff 125272.
sabel83 marked an inline comment as done.
https://reviews.llvm.org/D5767
Files:
include/clang/Driver/CC1Options.td
include/clang/Frontend/FrontendActions.h
include/clang/Frontend/FrontendOptions.h
include/clang/FrontendTool/Utils.h
include/c
xazax.hun accepted this revision.
xazax.hun added a comment.
I found some nit, otherwise LG!
I think you should includ the context in the patches, that makes them reviewing
much easier. See:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
Com
sabel83 updated this revision to Diff 123465.
Herald added a subscriber: rnkovacs.
https://reviews.llvm.org/D5767
Files:
include/clang/Driver/CC1Options.td
include/clang/Frontend/FrontendActions.h
include/clang/Frontend/FrontendOptions.h
include/clang/FrontendTool/Utils.h
include/clang/
xazax.hun added inline comments.
Comment at: include/clang/Sema/TemplateInstCallback.h:44
+template
+void initialize(TemplateInstantiationCallbackPtrs& Callbacks_, const Sema
&TheSema)
+{
Nit: this file should be clang formatted, it does not follow the LLVM st
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D5767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co
sabel83 added a comment.
In https://reviews.llvm.org/D5767#920085, @rsmith wrote:
> I'm not entirely sure what's happening with this and
> https://reviews.llvm.org/D38818, but the direction looks good to me, and I
> left a couple of comments on the other review thread.
This was the original p
sabel83 updated this revision to Diff 122325.
sabel83 marked an inline comment as done.
https://reviews.llvm.org/D5767
Files:
include/clang/Driver/CC1Options.td
include/clang/Frontend/FrontendActions.h
include/clang/Frontend/FrontendOptions.h
include/clang/FrontendTool/Utils.h
include/c
On Thu, Nov 9, 2017 at 4:02 AM Richard Smith - zygoloid via Phabricator <
revi...@reviews.llvm.org> wrote:
> rsmith added a comment.
>
> I'm not entirely sure what's happening with this and
> https://reviews.llvm.org/D38818, but the direction looks good to me, and
> I left a couple of comments on
rsmith added a comment.
I'm not entirely sure what's happening with this and
https://reviews.llvm.org/D38818, but the direction looks good to me, and I left
a couple of comments on the other review thread.
https://reviews.llvm.org/D5767
___
cfe-co
klimek added a comment.
Weekly ping. This week is C++ committee, so it's again going to be hard to get
Richards attention :( It's an unfortunate time of year to land major
API-touching changes.
https://reviews.llvm.org/D5767
___
cfe-commits mailin
klimek added a comment.
Just FYI, I talked with Richard and he'll not get to it before next week. Hope
that's fine.
https://reviews.llvm.org/D5767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinf
sabel83 updated this revision to Diff 120358.
sabel83 marked 2 inline comments as done.
https://reviews.llvm.org/D5767
Files:
include/clang/Driver/CC1Options.td
include/clang/Frontend/FrontendActions.h
include/clang/Frontend/FrontendOptions.h
include/clang/FrontendTool/Utils.h
include/c
klimek added a comment.
Ok, I think this is starting to look really good. Now we should try to grab
Richard's attention to make sure it's fine to submit with the current set of
FIXMEs.
Comment at: lib/Frontend/FrontendActions.cpp:386
+ // This part is normally done by ASTFro
sabel83 added a comment.
I've updated the patch based on your comments.
https://reviews.llvm.org/D5767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
sabel83 updated this revision to Diff 119774.
https://reviews.llvm.org/D5767
Files:
include/clang/Driver/CC1Options.td
include/clang/Frontend/FrontendActions.h
include/clang/Frontend/FrontendOptions.h
include/clang/FrontendTool/Utils.h
include/clang/Sema/Sema.h
include/clang/Sema/Temp
klimek added a comment.
Whee, thanks for driving this, much appreciated :D
Comment at: include/clang/FrontendTool/Utils.h:25
+
+std::unique_ptr CreateFrontendAction(CompilerInstance &CI);
Please add a comment now that this is exported.
Comm
sabel83 updated this revision to Diff 119377.
sabel83 added reviewers: cfe-commits, klimek, mclow.lists, martong, xazax.hun.
https://reviews.llvm.org/D5767
Files:
include/clang/Driver/CC1Options.td
include/clang/Frontend/FrontendActions.h
include/clang/Frontend/FrontendOptions.h
include/c
klimek added a comment.
Ok, just let me know when you have an updated patch out for review for
submission, so I can make sure we'll get the reviews done in a timely fashion.
I really want to see this go in :)
http://reviews.llvm.org/D5767
___
cfe-
sabel83 added a comment.
Hi Manuel,
The patch is maintained in the Templight git repository
(https://github.com/mikael-s-persson/templight/blob/master/templight_clang_patch.diff).
I've written some tests for it (based on our discussion at CppCon), they are
under review at the moment (see
http
klimek added a subscriber: klimek.
klimek added a comment.
Is this still the most current patch out there?
http://reviews.llvm.org/D5767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi
35 matches
Mail list logo
