zturner abandoned this revision.
zturner added a comment.
Once I split the JSONCompilationDatabase patch out, then the rest is strictly a
driver-only patch, and the other one is strictly a tooling patch. So I will
abandon this one and upload two new patches with a more targeted set of
reviewer
alexfh added a comment.
The changes in Driver LGTM (though I'd prefer someone who knows Driver to look).
Could you move lib/Tooling/JSONCompilationDatabase.cpp to a separate patch? It
seems like an orthogonal change.
As for a clang-cl-mode test for clang-tidy, clang-tidy-run-with-database.cpp i
zturner updated this revision to Diff 67751.
zturner added a comment.
I added a test for the clang Driver changes.
It seems I'm still having some trouble writing the clang-cl test for
clang-tidy. It depends on having a different compilation database that uses
clang-cl instead of clang. I foun
zturner added a comment.
In https://reviews.llvm.org/D23409#513017, @alexfh wrote:
> May I ask you to upload patches with full diff context next time? I know,
> it's not directly supported by TortoiseGit, but there are at least two other
> reasonable ways of generating full diffs for Phabricato
alexfh added a comment.
May I ask you to upload patches with full diff context next time? I know, it's
not directly supported by TortoiseGit, but there are at least two other
reasonable ways of generating full diffs for Phabricator:
http://llvm.org/docs/Phabricator.html#requesting-a-review-via-
alexfh added a comment.
In https://reviews.llvm.org/D23409#512819, @zturner wrote:
> In https://reviews.llvm.org/D23409#512745, @zturner wrote:
>
> > In https://reviews.llvm.org/D23409#512720, @aaron.ballman wrote:
> >
> > > Patch generally LGTM, though I wonder if there's a way we can add test
zturner added a comment.
In https://reviews.llvm.org/D23409#512745, @zturner wrote:
> In https://reviews.llvm.org/D23409#512720, @aaron.ballman wrote:
>
> > Patch generally LGTM, though I wonder if there's a way we can add test
> > coverage for this.
>
>
> I'd imagine we can just enable the clan
zturner added inline comments.
Comment at: lib/Driver/Driver.cpp:93
@@ +92,3 @@
+ ArrayRef Args) {
+ auto Default = ToolChain::getTargetAndModeFromProgramName(ProgramName);
+ StringRef DefaultMode(Default.second);
rnk wrote:
> Why not
zturner added inline comments.
Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
@@ -115,1 +118,3 @@
StringRef EscapedCommandLine) {
+#if defined(LLVM_ON_WIN32)
+ llvm::BumpPtrAllocator Alloc;
rnk wrote:
> It would be nice if the JSON file just told us
zturner added a comment.
In https://reviews.llvm.org/D23409#512720, @aaron.ballman wrote:
> Patch generally LGTM, though I wonder if there's a way we can add test
> coverage for this.
I'd imagine we can just enable the clang-tidy test suite on Windows (I'm
assuming it's currently disabled).
rnk added inline comments.
Comment at: lib/Driver/Driver.cpp:93
@@ +92,3 @@
+ ArrayRef Args) {
+ auto Default = ToolChain::getTargetAndModeFromProgramName(ProgramName);
+ StringRef DefaultMode(Default.second);
Why not change ToolInvoc
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a comment.
Patch generally LGTM, though I wonder if there's a way we can add test coverage
for this.
https://reviews.llvm.org/D23409
___
cfe-commits mailing list
cfe-commits@lists
12 matches
Mail list logo
