Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
I'll try and figure out who that was on Monday and loop them in. I'm not
sure what problems the previous person ran into, but the test suite passes,
i can run it on a large codebase, and all the results look fine and as if
the tool is working. Hopefully the previous person has more insight into
what I should be looking for.
It's not that it's time critical, I'm just a little surprised that it seems
so important to support a use case that I'm not sure anyone has ever tried
to do or would ever want to do.
The idea of copying a compilation database around across machines, is this
really something we need to go out of our way to support?
On Sat, Aug 13, 2016 at 11:34 PM Manuel Klimek wrote:
> For years nobody worked on Windows support, so I'm somewhat surprised this
> is suddenly time critical.
> Usually the way this works is that we add the feature to upstream cmake,
> and then make a recent enough cmake a requirement for tooling. There's no
> need to make it a requirement for anything else. That has worked well for
> the initial version, too.
>
> My main problem is that I'm surprised you say you got a working version at
> all, given all the differences. I'm on vacation, but afterwards I'm happy
> to look up who previously worked on this and loop them into this thread. Or
> you can figure out who that was (they added the arguments support iirc) and
> make sure they're signing of on this.
>
>
>
> On Sun, Aug 14, 2016, 8:52 AM Zachary Turner wrote:
>
>> According to the existing spec [
>> http://clang.llvm.org/docs/JSONCompilationDatabase.html], the command
>> field "must be a valid command to rerun the exact compilation step for the
>> translation unit in the environment the build system uses.".
>>
>> So copying compilation databases across environments with incompatible
>> command line syntaxes is already against spec.
>>
>> That said, this does make me think that perhaps we should be checking the
>> target triple instead of the host compilation environment, because if you
>> were to cross compile clang-tidy for Windows from linux then try to use
>> that clang-tidy on Windows, it would expect unix paths.
>>
>> How does that sound?
>> On Sat, Aug 13, 2016 at 10:31 PM Zachary Turner
>> wrote:
>>
>>> Also, compilation database support with CMake works correctly on
>>> Windows. It generates Windows command lines which need to be tokenized
>>> using Windows command line rules (hence why this patch makes clang-tidy
>>> work on Windows)
>>> On Sat, Aug 13, 2016 at 10:30 PM Zachary Turner
>>> wrote:
>>>
I'm not disagreeing that it would be nice if CMake supported this. It's
probably worth trying to do even.
But do we want to block having a working clang-tidy for clang-cl for
months because of that? Even if we can upstream the patch, how long before
we up the minimum required CMake version?
The solution here does not regress behavior on any supported
configuration, and adds support for a new platform. Copying a compilation
database from one machine to another seems like a hypothetical edge case.
To support testing perhaps we can make our compilation database parser
support an optional field in the json that CMake doesn't know about like
PathSyntax: 'windows'. Again, this seems like something we could do
iteratively and with low priority because it's not needed in order to
enable or fix any presently supported use cases.
On Sat, Aug 13, 2016 at 9:58 PM Manuel Klimek
wrote:
>
>
> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner
> wrote:
>
>> The json is generated by CMake, so I don't thinks we can do this
>> without patching CMake, which is a non-starter.
>>
>
> Why? We did add compilation database support to cmake in the first
> place. Back then I did not support windows, btw, so I'd be surprised if
> that worked now without also using the arguments format that has already
> been added to the spec for that reason.
>
> I don't think this will break mingw. Mingw is still Windows, and still
>> uses Windows backslashes, quoting rules, and escaping rules.
>>
>> You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not
>> defined.
>>
>> Reid, do you agree with this?
>>
>
> I'd like to see a stronger reason why adding arguments support to
> cmake is not the right thing to do.
>
>
>> On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko <
>> ale...@google.com> wrote:
>>
>>> alexfh added inline comments.
>>>
>>>
>>> Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
>>> @@ -115,1 +118,3 @@
>>> StringRef EscapedCommandLine) {
>>> +#if defined(LLVM_ON_WIN32)
>>> + llvm::BumpPtrAllocator Alloc;
>>>
>>> As noted on D23409, this will likely break mingw compatibility in
>>> clang on windows. We should either add a sort
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
For years nobody worked on Windows support, so I'm somewhat surprised this
is suddenly time critical.
Usually the way this works is that we add the feature to upstream cmake,
and then make a recent enough cmake a requirement for tooling. There's no
need to make it a requirement for anything else. That has worked well for
the initial version, too.
My main problem is that I'm surprised you say you got a working version at
all, given all the differences. I'm on vacation, but afterwards I'm happy
to look up who previously worked on this and loop them into this thread. Or
you can figure out who that was (they added the arguments support iirc) and
make sure they're signing of on this.
On Sun, Aug 14, 2016, 8:52 AM Zachary Turner wrote:
> According to the existing spec [
> http://clang.llvm.org/docs/JSONCompilationDatabase.html], the command
> field "must be a valid command to rerun the exact compilation step for the
> translation unit in the environment the build system uses.".
>
> So copying compilation databases across environments with incompatible
> command line syntaxes is already against spec.
>
> That said, this does make me think that perhaps we should be checking the
> target triple instead of the host compilation environment, because if you
> were to cross compile clang-tidy for Windows from linux then try to use
> that clang-tidy on Windows, it would expect unix paths.
>
> How does that sound?
> On Sat, Aug 13, 2016 at 10:31 PM Zachary Turner
> wrote:
>
>> Also, compilation database support with CMake works correctly on Windows.
>> It generates Windows command lines which need to be tokenized using Windows
>> command line rules (hence why this patch makes clang-tidy work on Windows)
>> On Sat, Aug 13, 2016 at 10:30 PM Zachary Turner
>> wrote:
>>
>>> I'm not disagreeing that it would be nice if CMake supported this. It's
>>> probably worth trying to do even.
>>>
>>> But do we want to block having a working clang-tidy for clang-cl for
>>> months because of that? Even if we can upstream the patch, how long before
>>> we up the minimum required CMake version?
>>>
>>> The solution here does not regress behavior on any supported
>>> configuration, and adds support for a new platform. Copying a compilation
>>> database from one machine to another seems like a hypothetical edge case.
>>>
>>> To support testing perhaps we can make our compilation database parser
>>> support an optional field in the json that CMake doesn't know about like
>>> PathSyntax: 'windows'. Again, this seems like something we could do
>>> iteratively and with low priority because it's not needed in order to
>>> enable or fix any presently supported use cases.
>>> On Sat, Aug 13, 2016 at 9:58 PM Manuel Klimek wrote:
>>>
On Sat, Aug 13, 2016, 10:17 PM Zachary Turner
wrote:
> The json is generated by CMake, so I don't thinks we can do this
> without patching CMake, which is a non-starter.
>
Why? We did add compilation database support to cmake in the first
place. Back then I did not support windows, btw, so I'd be surprised if
that worked now without also using the arguments format that has already
been added to the spec for that reason.
I don't think this will break mingw. Mingw is still Windows, and still
> uses Windows backslashes, quoting rules, and escaping rules.
>
> You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not
> defined.
>
> Reid, do you agree with this?
>
I'd like to see a stronger reason why adding arguments support to cmake
is not the right thing to do.
> On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko <
> ale...@google.com> wrote:
>
>> alexfh added inline comments.
>>
>>
>> Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
>> @@ -115,1 +118,3 @@
>> StringRef EscapedCommandLine) {
>> +#if defined(LLVM_ON_WIN32)
>> + llvm::BumpPtrAllocator Alloc;
>>
>> As noted on D23409, this will likely break mingw compatibility in
>> clang on windows. We should either add a sort of auto-detection of the
>> command line format, or extend the JSON compilation database with a way
>> to
>> specify the command line format used (or as Reid suggests, specify a list
>> of arguments, but this should be done in a backward-compatible way).
>>
>>
>> https://reviews.llvm.org/D23455
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23476: Allow manual specification of the binary directory in check_clang_tidy.py
The script was never intended to be run manually. Its interface is rather inconvenient for this. If you need to run a single test, it's easier to do using llvm-lit (`path/to/lit test/file.cpp`). But if you for some reason want to run the script directly, it should be possible to just modify PATH or switch to the bin directory of the build. I don't think this flag is needed. On Aug 13, 2016 21:14, "Zachary Turner" wrote: > I was trying to rerun the lit commands manually to diagnose a failing > test. This all works fine from lit, but if you want to run > check_clang_tidy.py from the command line, this is convenient > On Sat, Aug 13, 2016 at 10:27 AM Alexander Kornienko > wrote: > >> alexfh added a comment. >> >> Why is this change needed? What exactly doesn't work without it? I think, >> lit should take care of setting paths or changing directories appropriately. >> >> >> https://reviews.llvm.org/D23476 >> >> >> >> ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
According to the existing spec [
http://clang.llvm.org/docs/JSONCompilationDatabase.html], the command field
"must be a valid command to rerun the exact compilation step for the
translation unit in the environment the build system uses.".
So copying compilation databases across environments with incompatible
command line syntaxes is already against spec.
That said, this does make me think that perhaps we should be checking the
target triple instead of the host compilation environment, because if you
were to cross compile clang-tidy for Windows from linux then try to use
that clang-tidy on Windows, it would expect unix paths.
How does that sound?
On Sat, Aug 13, 2016 at 10:31 PM Zachary Turner wrote:
> Also, compilation database support with CMake works correctly on Windows.
> It generates Windows command lines which need to be tokenized using Windows
> command line rules (hence why this patch makes clang-tidy work on Windows)
> On Sat, Aug 13, 2016 at 10:30 PM Zachary Turner
> wrote:
>
>> I'm not disagreeing that it would be nice if CMake supported this. It's
>> probably worth trying to do even.
>>
>> But do we want to block having a working clang-tidy for clang-cl for
>> months because of that? Even if we can upstream the patch, how long before
>> we up the minimum required CMake version?
>>
>> The solution here does not regress behavior on any supported
>> configuration, and adds support for a new platform. Copying a compilation
>> database from one machine to another seems like a hypothetical edge case.
>>
>> To support testing perhaps we can make our compilation database parser
>> support an optional field in the json that CMake doesn't know about like
>> PathSyntax: 'windows'. Again, this seems like something we could do
>> iteratively and with low priority because it's not needed in order to
>> enable or fix any presently supported use cases.
>> On Sat, Aug 13, 2016 at 9:58 PM Manuel Klimek wrote:
>>
>>>
>>>
>>> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner
>>> wrote:
>>>
The json is generated by CMake, so I don't thinks we can do this
without patching CMake, which is a non-starter.
>>>
>>> Why? We did add compilation database support to cmake in the first
>>> place. Back then I did not support windows, btw, so I'd be surprised if
>>> that worked now without also using the arguments format that has already
>>> been added to the spec for that reason.
>>>
>>> I don't think this will break mingw. Mingw is still Windows, and still
uses Windows backslashes, quoting rules, and escaping rules.
You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not
defined.
Reid, do you agree with this?
>>>
>>> I'd like to see a stronger reason why adding arguments support to cmake
>>> is not the right thing to do.
>>>
>>>
On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko
wrote:
> alexfh added inline comments.
>
>
> Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
> @@ -115,1 +118,3 @@
> StringRef EscapedCommandLine) {
> +#if defined(LLVM_ON_WIN32)
> + llvm::BumpPtrAllocator Alloc;
>
> As noted on D23409, this will likely break mingw compatibility in
> clang on windows. We should either add a sort of auto-detection of the
> command line format, or extend the JSON compilation database with a way to
> specify the command line format used (or as Reid suggests, specify a list
> of arguments, but this should be done in a backward-compatible way).
>
>
> https://reviews.llvm.org/D23455
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
Also, compilation database support with CMake works correctly on Windows.
It generates Windows command lines which need to be tokenized using Windows
command line rules (hence why this patch makes clang-tidy work on Windows)
On Sat, Aug 13, 2016 at 10:30 PM Zachary Turner wrote:
> I'm not disagreeing that it would be nice if CMake supported this. It's
> probably worth trying to do even.
>
> But do we want to block having a working clang-tidy for clang-cl for
> months because of that? Even if we can upstream the patch, how long before
> we up the minimum required CMake version?
>
> The solution here does not regress behavior on any supported
> configuration, and adds support for a new platform. Copying a compilation
> database from one machine to another seems like a hypothetical edge case.
>
> To support testing perhaps we can make our compilation database parser
> support an optional field in the json that CMake doesn't know about like
> PathSyntax: 'windows'. Again, this seems like something we could do
> iteratively and with low priority because it's not needed in order to
> enable or fix any presently supported use cases.
> On Sat, Aug 13, 2016 at 9:58 PM Manuel Klimek wrote:
>
>>
>>
>> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner wrote:
>>
>>> The json is generated by CMake, so I don't thinks we can do this without
>>> patching CMake, which is a non-starter.
>>>
>>
>> Why? We did add compilation database support to cmake in the first place.
>> Back then I did not support windows, btw, so I'd be surprised if that
>> worked now without also using the arguments format that has already been
>> added to the spec for that reason.
>>
>> I don't think this will break mingw. Mingw is still Windows, and still
>>> uses Windows backslashes, quoting rules, and escaping rules.
>>>
>>> You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not
>>> defined.
>>>
>>> Reid, do you agree with this?
>>>
>>
>> I'd like to see a stronger reason why adding arguments support to cmake
>> is not the right thing to do.
>>
>>
>>> On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko
>>> wrote:
>>>
alexfh added inline comments.
Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
@@ -115,1 +118,3 @@
StringRef EscapedCommandLine) {
+#if defined(LLVM_ON_WIN32)
+ llvm::BumpPtrAllocator Alloc;
As noted on D23409, this will likely break mingw compatibility in clang
on windows. We should either add a sort of auto-detection of the command
line format, or extend the JSON compilation database with a way to specify
the command line format used (or as Reid suggests, specify a list of
arguments, but this should be done in a backward-compatible way).
https://reviews.llvm.org/D23455
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
I'm not disagreeing that it would be nice if CMake supported this. It's
probably worth trying to do even.
But do we want to block having a working clang-tidy for clang-cl for months
because of that? Even if we can upstream the patch, how long before we up
the minimum required CMake version?
The solution here does not regress behavior on any supported configuration,
and adds support for a new platform. Copying a compilation database from
one machine to another seems like a hypothetical edge case.
To support testing perhaps we can make our compilation database parser
support an optional field in the json that CMake doesn't know about like
PathSyntax: 'windows'. Again, this seems like something we could do
iteratively and with low priority because it's not needed in order to
enable or fix any presently supported use cases.
On Sat, Aug 13, 2016 at 9:58 PM Manuel Klimek wrote:
>
>
> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner wrote:
>
>> The json is generated by CMake, so I don't thinks we can do this without
>> patching CMake, which is a non-starter.
>>
>
> Why? We did add compilation database support to cmake in the first place.
> Back then I did not support windows, btw, so I'd be surprised if that
> worked now without also using the arguments format that has already been
> added to the spec for that reason.
>
> I don't think this will break mingw. Mingw is still Windows, and still
>> uses Windows backslashes, quoting rules, and escaping rules.
>>
>> You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not
>> defined.
>>
>> Reid, do you agree with this?
>>
>
> I'd like to see a stronger reason why adding arguments support to cmake is
> not the right thing to do.
>
>
>> On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko
>> wrote:
>>
>>> alexfh added inline comments.
>>>
>>>
>>> Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
>>> @@ -115,1 +118,3 @@
>>> StringRef EscapedCommandLine) {
>>> +#if defined(LLVM_ON_WIN32)
>>> + llvm::BumpPtrAllocator Alloc;
>>>
>>> As noted on D23409, this will likely break mingw compatibility in clang
>>> on windows. We should either add a sort of auto-detection of the command
>>> line format, or extend the JSON compilation database with a way to specify
>>> the command line format used (or as Reid suggests, specify a list of
>>> arguments, but this should be done in a backward-compatible way).
>>>
>>>
>>> https://reviews.llvm.org/D23455
>>>
>>>
>>>
>>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
On Sat, Aug 13, 2016, 10:17 PM Zachary Turner wrote:
> The json is generated by CMake, so I don't thinks we can do this without
> patching CMake, which is a non-starter.
>
Why? We did add compilation database support to cmake in the first place.
Back then I did not support windows, btw, so I'd be surprised if that
worked now without also using the arguments format that has already been
added to the spec for that reason.
I don't think this will break mingw. Mingw is still Windows, and still uses
> Windows backslashes, quoting rules, and escaping rules.
>
> You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not
> defined.
>
> Reid, do you agree with this?
>
I'd like to see a stronger reason why adding arguments support to cmake is
not the right thing to do.
> On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko
> wrote:
>
>> alexfh added inline comments.
>>
>>
>> Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
>> @@ -115,1 +118,3 @@
>> StringRef EscapedCommandLine) {
>> +#if defined(LLVM_ON_WIN32)
>> + llvm::BumpPtrAllocator Alloc;
>>
>> As noted on D23409, this will likely break mingw compatibility in clang
>> on windows. We should either add a sort of auto-detection of the command
>> line format, or extend the JSON compilation database with a way to specify
>> the command line format used (or as Reid suggests, specify a list of
>> arguments, but this should be done in a backward-compatible way).
>>
>>
>> https://reviews.llvm.org/D23455
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix
Prazek updated this revision to Diff 67967.
Prazek added a comment.
- fixes
https://reviews.llvm.org/D23343
Files:
clang-tidy/modernize/MakeSmartPtrCheck.cpp
docs/ReleaseNotes.rst
test/clang-tidy/modernize-make-shared.cpp
test/clang-tidy/modernize-make-unique.cpp
Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -103,6 +103,38 @@
std::unique_ptr Placement = std::unique_ptr(new (PInt) int{3});
}
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+ Private(int z) {}
+
+public:
+ Private() {}
+ void create() {
+auto callsPublic = std::unique_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique();
+auto ptr = std::unique_ptr(new Private(42));
+ }
+
+ virtual ~Private();
+};
+
+class Protected {
+protected:
+ Protected() {}
+
+public:
+ Protected(int, int) {}
+ void create() {
+auto callsPublic = std::unique_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique(1, 2);
+auto ptr = std::unique_ptr(new Protected);
+ }
+};
+
void initialization(int T, Base b) {
// Test different kinds of initialization of the pointee.
Index: test/clang-tidy/modernize-make-shared.cpp
===
--- test/clang-tidy/modernize-make-shared.cpp
+++ test/clang-tidy/modernize-make-shared.cpp
@@ -100,6 +100,38 @@
std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
}
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+ Private(int z) {}
+
+public:
+ Private() {}
+ void create() {
+auto callsPublic = std::shared_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared();
+auto ptr = std::shared_ptr(new Private(42));
+ }
+
+ virtual ~Private();
+};
+
+class Protected {
+protected:
+ Protected() {}
+
+public:
+ Protected(int, int) {}
+ void create() {
+auto callsPublic = std::shared_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared(1, 2);
+auto ptr = std::shared_ptr(new Protected);
+ }
+};
+
void initialization(int T, Base b) {
// Test different kinds of initialization of the pointee.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -85,6 +85,15 @@
Warns about the performance overhead arising from concatenating strings using
the ``operator+``, instead of ``operator+=``.
+
+Fixed bugs:
+- `modernize-make-unique
+ `_
+ and `modernize-make-shared
+ `_
+ Calling ``make_{unique|shared}`` from within a member function of a type
+ with a private or protected constructor would be ill-formed.
+
Improvements to include-fixer
-
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -29,13 +29,19 @@
if (!getLangOpts().CPlusPlus11)
return;
+ // Calling make_smart_ptr from within a member function of a type with a
+ // private or protected constructor would be ill-formed.
+ auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(
+ hasDeclaration(decl(anyOf(isPrivate(), isProtected(;
+
Finder->addMatcher(
cxxBindTemporaryExpr(has(ignoringParenImpCasts(
cxxConstructExpr(
hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
hasArgument(0,
cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
- equalsBoundNode(PointerType))
+ equalsBoundNode(PointerType),
+ CanCallCtor)
.bind(NewExpression)))
.bind(ConstructorCall,
this);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix
Prazek updated this revision to Diff 67966.
Prazek marked 6 inline comments as done.
Prazek added a comment.
- fixes
https://reviews.llvm.org/D23343
Files:
clang-tidy/modernize/MakeSmartPtrCheck.cpp
docs/ReleaseNotes.rst
test/clang-tidy/modernize-make-shared.cpp
test/clang-tidy/modernize-make-unique.cpp
Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -103,6 +103,36 @@
std::unique_ptr Placement = std::unique_ptr(new (PInt) int{3});
}
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+ Private(int z) {}
+public:
+ Private() {}
+ void create() {
+auto callsPublic = std::unique_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique();
+auto ptr = std::unique_ptr(new Private(42));
+ }
+
+ virtual ~Private();
+};
+
+class Protected {
+protected:
+ Protected() {}
+public:
+ Protected(int, int) {}
+ void create() {
+auto callsPublic = std::unique_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique(1, 2);
+auto ptr = std::unique_ptr(new Protected);
+ }
+};
+
void initialization(int T, Base b) {
// Test different kinds of initialization of the pointee.
Index: test/clang-tidy/modernize-make-shared.cpp
===
--- test/clang-tidy/modernize-make-shared.cpp
+++ test/clang-tidy/modernize-make-shared.cpp
@@ -100,6 +100,36 @@
std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
}
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+ Private(int z) {}
+public:
+ Private() {}
+ void create() {
+auto callsPublic = std::shared_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared();
+auto ptr = std::shared_ptr(new Private(42));
+ }
+
+ virtual ~Private();
+};
+
+class Protected {
+protected:
+ Protected() {}
+public:
+ Protected(int, int) {}
+ void create() {
+auto callsPublic = std::shared_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared(1, 2);
+auto ptr = std::shared_ptr(new Protected);
+ }
+};
+
void initialization(int T, Base b) {
// Test different kinds of initialization of the pointee.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -85,6 +85,15 @@
Warns about the performance overhead arising from concatenating strings using
the ``operator+``, instead of ``operator+=``.
+
+Fixed bugs:
+- `modernize-make-unique
+ `_
+ and `modernize-make-shared
+ `_
+ Calling ``make_{unique|shared}`` from within a member function of a type
+ with a private or protected constructor would be ill-formed.
+
Improvements to include-fixer
-
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -29,13 +29,21 @@
if (!getLangOpts().CPlusPlus11)
return;
+ // Calling make_smart_ptr from within a member function of a type with a
+ // private or protected constructor would be ill-formed.
+ auto CanCallCtor = unless(has(
+ ignoringImpCasts(cxxConstructExpr(hasDeclaration(decl(anyOf(isPrivate()
+, isProtected(
+ ;
+
Finder->addMatcher(
cxxBindTemporaryExpr(has(ignoringParenImpCasts(
cxxConstructExpr(
hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
hasArgument(0,
cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
- equalsBoundNode(PointerType))
+ equalsBoundNode(PointerType),
+ CanCallCtor)
.bind(NewExpression)))
.bind(ConstructorCall,
this);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23434: Don't allow llvm-include-order to intermingle includes from different files.
On Fri, Aug 12, 2016 at 8:22 PM, Zachary Turner wrote: > Sounds good. Just to be clear, you plan to delete the code from > clang-tidy, then take the code from clang-format and move it to clang-tidy, > and have clang-format call clang-tidy (or otherwise share the code somehow > so they both use the same implementation)? > No. clang-tidy just calls into clang-format (the library, not the binary) and if clang-format would change the order of #includes, then clang-tidy will display a warning. I already have implemented this. Will try to clean it up and send out a patch. I may still try to implement cross-block reordering in clang-tidy because > it's the only way to do it in such a way that it just warns you but doesn't > apply fixits, or applies them only if it doesn't break the compile, which > is what I need at the moment. But since this is just experimental anyway, > it probably shouldn't matter much. And if I end up getting something that > works reasonably well, we can always move that code over to clang-format if > we want to support cross-block reordering there. > It's highly unlikely that we'll be able to share code here. The way #includes are detected and handled are very different between clang-format and clang-tidy. If you want to try to implement this for clang-format, by all means. If you are thinking of implementing this for clang-tidy, just take what we have tried before. I can send you pointers to the code (although it has never made it to upstream clang-tidy). On Fri, Aug 12, 2016 at 11:17 AM Daniel Jasper wrote: > >> I haven't read the patch, but if Alex is ok, so am I.. just wanted to >> make sure that we don't spend much more time on this, as we are likely >> going to remove most of the code.. >> >> On Aug 12, 2016 6:42 PM, "Zachary Turner" wrote: >> >>> Ahh, I see. Just to be clear, is there an LGTM to get this path in? I >>> know alexfh@ lgtm'ed it, want to make sure you're ok with this too. >>> >>> On Fri, Aug 12, 2016 at 9:40 AM Daniel Jasper >>> wrote: >>> The check's implementation will be replaced by a simple call to clang tidy. It will remain a check in clang tidy to continue to cater to both use cases. On Aug 12, 2016 6:19 PM, "Zachary Turner" wrote: > That's actually the reason I think it makes more sense in clang tidy. > It can be a configuration option, off by default, and since there is more > control over whether to apply fixits, and it doesn't apply fixits by > default, it would be easier to iterate on the experimental nature of it > without messing up code > > > On Fri, Aug 12, 2016 at 9:14 AM Alexander Kornienko > wrote: > >> alexfh added a comment. >> >> In https://reviews.llvm.org/D23434#513839, @djasper wrote: >> >> > I think we got confused. We once had tried to write an experimental >> separate check to comply with Google's style guide. If you want to fiddle >> around with that, contact me, I can send you pointers. But as I mentioned >> we moved away from that. And I think it makes more sense to re-create the >> sort-across-blocks functionality in clang-format and not in clang-tidy. >> >> >> Yep, we definitely got confused. That experimental check actually >> implemented cross-block sorting, but this caused a bunch of issues. Which >> makes me think that proper implementation of cross-block include sorting >> is >> challenging be it in clang-format or clang-tidy. Clang-format probably >> makes it even more complex, since a higher safety of transformations is >> expected from it. >> >> >> https://reviews.llvm.org/D23434 >> >> >> >> ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
The only thing that won't work is generating a compilation database on one
machine and physically copying it to another machine, but I don't think we
should worry about that. We can try to submit a request to CMake to get
more info in the compilation database, but something like that is months
out, if they do it all
On Sat, Aug 13, 2016 at 12:17 PM Zachary Turner wrote:
> The json is generated by CMake, so I don't thinks we can do this without
> patching CMake, which is a non-starter.
>
> I don't think this will break mingw. Mingw is still Windows, and still
> uses Windows backslashes, quoting rules, and escaping rules.
>
> You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not
> defined.
>
> Reid, do you agree with this?
>
> On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko
> wrote:
>
>> alexfh added inline comments.
>>
>>
>> Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
>> @@ -115,1 +118,3 @@
>> StringRef EscapedCommandLine) {
>> +#if defined(LLVM_ON_WIN32)
>> + llvm::BumpPtrAllocator Alloc;
>>
>> As noted on D23409, this will likely break mingw compatibility in clang
>> on windows. We should either add a sort of auto-detection of the command
>> line format, or extend the JSON compilation database with a way to specify
>> the command line format used (or as Reid suggests, specify a list of
>> arguments, but this should be done in a backward-compatible way).
>>
>>
>> https://reviews.llvm.org/D23455
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
The json is generated by CMake, so I don't thinks we can do this without
patching CMake, which is a non-starter.
I don't think this will break mingw. Mingw is still Windows, and still uses
Windows backslashes, quoting rules, and escaping rules.
You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not
defined.
Reid, do you agree with this?
On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko
wrote:
> alexfh added inline comments.
>
>
> Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
> @@ -115,1 +118,3 @@
> StringRef EscapedCommandLine) {
> +#if defined(LLVM_ON_WIN32)
> + llvm::BumpPtrAllocator Alloc;
>
> As noted on D23409, this will likely break mingw compatibility in clang on
> windows. We should either add a sort of auto-detection of the command line
> format, or extend the JSON compilation database with a way to specify the
> command line format used (or as Reid suggests, specify a list of arguments,
> but this should be done in a backward-compatible way).
>
>
> https://reviews.llvm.org/D23455
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23476: Allow manual specification of the binary directory in check_clang_tidy.py
I was trying to rerun the lit commands manually to diagnose a failing test. This all works fine from lit, but if you want to run check_clang_tidy.py from the command line, this is convenient On Sat, Aug 13, 2016 at 10:27 AM Alexander Kornienko wrote: > alexfh added a comment. > > Why is this change needed? What exactly doesn't work without it? I think, > lit should take care of setting paths or changing directories appropriately. > > > https://reviews.llvm.org/D23476 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22130: Link static PIE programs against rcrt0.o on OpenBSD
joerg added a comment. This is missing a test case. https://reviews.llvm.org/D22130 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23471: [Documentation] Improve checks groups descriptions in clang-tidy/index.rst
alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. Awesome! Looks good! Repository: rL LLVM https://reviews.llvm.org/D23471 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows
alexfh added inline comments.
Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
@@ -115,1 +118,3 @@
StringRef EscapedCommandLine) {
+#if defined(LLVM_ON_WIN32)
+ llvm::BumpPtrAllocator Alloc;
As noted on D23409, this will likely break mingw compatibility in clang on
windows. We should either add a sort of auto-detection of the command line
format, or extend the JSON compilation database with a way to specify the
command line format used (or as Reid suggests, specify a list of arguments, but
this should be done in a backward-compatible way).
https://reviews.llvm.org/D23455
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23476: Allow manual specification of the binary directory in check_clang_tidy.py
alexfh added a comment. Why is this change needed? What exactly doesn't work without it? I think, lit should take care of setting paths or changing directories appropriately. https://reviews.llvm.org/D23476 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23480: Add a test for clang-tidy using the clang cl driver
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
LG with two nits.
> In order to get this to work, the positional arguments must use
> --driver-mode=cl , and NOT clang-cl
It's not that important for the fixed compilation database, as long as there is
a way to specify the driver mode. However, we need to make sure clang-tidy is
able to figure out correct driver mode from JSON compilation databases
generated on Windows for clang-cl builds. It should be a separate test though.
Comment at: test/clang-tidy/clang-cl-driver.cpp:1
@@ +1,2 @@
+// RUN: clang-tidy -checks=-*,modernize-use-nullptr %s -- --driver-mode=cl
/DTEST1 /DFOO=foo /DBAR=bar | FileCheck
-implicit-check-not="{{warning|error}}:" %s
+int *a = 0;
I wonder whether the `/Dname#value` format should work in clang-cl mode as well
(it's supported by cl.exe, if I understand
https://msdn.microsoft.com/en-us/library/hhzbb5c8.aspx correctly).
Comment at: test/clang-tidy/clang-cl-driver.cpp:18
@@ +17,1 @@
+#endif
\ No newline at end of file
Please fix the "No newline at end of file".
https://reviews.llvm.org/D23480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23485: [Branch 3.9] Remove any traces of partial constexpr lambda implementation (per Richard's request)
faisalv added a comment. Everything compiles - and all tests in clang/test pass. https://reviews.llvm.org/D23485 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D23485: [Branch 3.9] Remove any traces of partial constexpr lambda implementation (per Richard's request)
faisalv created this revision.
faisalv added reviewers: hans, rsmith.
faisalv added a subscriber: cfe-commits.
Per Richard's request here:
https://llvm.org/bugs/show_bug.cgi?id=28366#c10
This patch essentially reverses all the changes from the following commit:
https://reviews.llvm.org/rL264513 for the branch_3.9.
[This is my first time submitting 'pull requests' for a specific branch. I
checked-out the respective release branches for llvm and clang, and reverted
the above commit (resolved a conflict in the process) and created a patch. My
box is currently still compiling and running regression tests (will update the
revision within the next two hours if any revisions need to be made). Please
let me know if I should be doing anything different]
https://reviews.llvm.org/D23485
Files:
include/clang/Basic/DiagnosticASTKinds.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Sema/Sema.h
lib/AST/ExprConstant.cpp
lib/Parse/ParseExprCXX.cpp
lib/Sema/SemaLambda.cpp
lib/Sema/TreeTransform.h
test/Parser/cxx1z-constexpr-lambdas.cpp
test/SemaCXX/cxx1z-constexpr-lambdas.cpp
Index: test/SemaCXX/cxx1z-constexpr-lambdas.cpp
===
--- test/SemaCXX/cxx1z-constexpr-lambdas.cpp
+++ test/SemaCXX/cxx1z-constexpr-lambdas.cpp
@@ -1,36 +0,0 @@
-// RUN: %clang_cc1 -std=c++1z -verify -fsyntax-only -fblocks %s
-// RUN: %clang_cc1 -std=c++1z -verify -fsyntax-only -fblocks -fdelayed-template-parsing %s
-// RUN: %clang_cc1 -std=c++1z -verify -fsyntax-only -fblocks -fms-extensions %s
-// RUN: %clang_cc1 -std=c++1z -verify -fsyntax-only -fblocks -fdelayed-template-parsing -fms-extensions %s
-
-namespace test_constexpr_checking {
-
-namespace ns1 {
- struct NonLit { ~NonLit(); }; //expected-note{{not literal}}
- auto L = [](NonLit NL) constexpr { }; //expected-error{{not a literal type}}
-} // end ns1
-
-namespace ns2 {
- auto L = [](int I) constexpr { asm("non-constexpr"); }; //expected-error{{not allowed in constexpr function}}
-} // end ns1
-
-} // end ns test_constexpr_checking
-
-namespace test_constexpr_call {
-
-namespace ns1 {
- auto L = [](int I) { return I; };
- static_assert(L(3) == 3);
-} // end ns1
-namespace ns2 {
- auto L = [](auto a) { return a; };
- static_assert(L(3) == 3);
- static_assert(L(3.14) == 3.14);
-}
-namespace ns3 {
- auto L = [](auto a) { asm("non-constexpr"); return a; }; //expected-note{{declared here}}
- constexpr int I = //expected-error{{must be initialized by a constant expression}}
- L(3); //expected-note{{non-constexpr function}}
-}
-
-} // end ns test_constexpr_call
\ No newline at end of file
Index: test/Parser/cxx1z-constexpr-lambdas.cpp
===
--- test/Parser/cxx1z-constexpr-lambdas.cpp
+++ test/Parser/cxx1z-constexpr-lambdas.cpp
@@ -1,31 +0,0 @@
-// RUN: %clang_cc1 -std=c++1z %s -verify
-// RUN: %clang_cc1 -std=c++14 %s -verify
-// RUN: %clang_cc1 -std=c++11 %s -verify
-
-
-auto XL0 = [] constexpr { }; //expected-error{{requires '()'}} expected-error{{expected body}}
-auto XL1 = [] () mutable
- mutable //expected-error{{cannot appear multiple times}}
- mutable { }; //expected-error{{cannot appear multiple times}}
-
-#if __cplusplus > 201402L
-auto XL2 = [] () constexpr mutable constexpr { }; //expected-error{{cannot appear multiple times}}
-auto L = []() mutable constexpr { };
-auto L2 = []() constexpr { };
-auto L4 = []() constexpr mutable { };
-auto XL16 = [] () constexpr
- mutable
- constexpr //expected-error{{cannot appear multiple times}}
- mutable //expected-error{{cannot appear multiple times}}
- mutable //expected-error{{cannot appear multiple times}}
- constexpr //expected-error{{cannot appear multiple times}}
- constexpr //expected-error{{cannot appear multiple times}}
- { };
-
-#else
-auto L = []() mutable constexpr {return 0; }; //expected-warning{{is a C++1z extension}}
-auto L2 = []() constexpr { return 0;};//expected-warning{{is a C++1z extension}}
-auto L4 = []() constexpr mutable { return 0; }; //expected-warning{{is a C++1z extension}}
-#endif
-
-
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -10222,9 +10222,7 @@
CXXMethodDecl *NewCallOperator = getSema().startLambdaDefinition(
Class, E->getIntroducerRange(), NewCallOpTSI,
E->getCallOperator()->getLocEnd(),
- NewCallOpTSI->getTypeLoc().castAs().getParams(),
- E->getCallOperator()->isConstexpr());
-
+ NewCallOpTSI->getTypeLoc().castAs().getParams());
LSI->CallOperator = NewCallOperator;
getDerived().transformAttrs(E->getCallOperator(), NewCallOperator);
Index: lib/Sema/SemaLambda.cpp
=
