Re: [clang-tools-extra] r284233 - [clang-move] Add header guard for the new header.

[Tim Northover via cfe-commits]

On 17 October 2016 at 08:36, Haojian Wu  wrote:
> Sorry for the trouble and delay (I missed this email previously).

No worries. I thought that might be what had happened, the weekend
deluge can hide anything.

> Should be fixed in r284391.

Thanks.

Tim.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r284233 - [clang-move] Add header guard for the new header.

[Haojian Wu via cfe-commits]

Should be fixed in r284391.

On Mon, Oct 17, 2016 at 4:59 PM, Haojian Wu  wrote:

> Hello Tim,
>
> Sorry for the trouble and delay (I missed this email previously).
>
> I'm looking into it. Could you give me a link to the problematic buildbot?
>
> On Mon, Oct 17, 2016 at 3:44 PM, Tim Northover 
> wrote:
>
>> On 14 October 2016 at 13:33, Tim Northover 
>> wrote:
>> > On 14 October 2016 at 06:01, Haojian Wu via cfe-commits
>> >  wrote:
>> >> +  std::string GuardName(FileName);
>> >> +  if (IsHeader) {
>> >> +std::replace(GuardName.begin(), GuardName.end(), '/', '_');
>> >> +std::replace(GuardName.begin(), GuardName.end(), '.', '_');
>> >> +std::replace(GuardName.begin(), GuardName.end(), '-', '_');
>> >
>> > I think this is causing problems with one of our bots that has an '@'
>> > in a path it uses. In general it seems like a whitelist based on what
>> > a C token is would be better than a blacklist of characters.
>> >
>> > Could you take a look?
>>
>> Ping.
>>
>> Tim.
>>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r284233 - [clang-move] Add header guard for the new header.

[Haojian Wu via cfe-commits]

Hello Tim,

Sorry for the trouble and delay (I missed this email previously).

I'm looking into it. Could you give me a link to the problematic buildbot?

On Mon, Oct 17, 2016 at 3:44 PM, Tim Northover 
wrote:

> On 14 October 2016 at 13:33, Tim Northover 
> wrote:
> > On 14 October 2016 at 06:01, Haojian Wu via cfe-commits
> >  wrote:
> >> +  std::string GuardName(FileName);
> >> +  if (IsHeader) {
> >> +std::replace(GuardName.begin(), GuardName.end(), '/', '_');
> >> +std::replace(GuardName.begin(), GuardName.end(), '.', '_');
> >> +std::replace(GuardName.begin(), GuardName.end(), '-', '_');
> >
> > I think this is causing problems with one of our bots that has an '@'
> > in a path it uses. In general it seems like a whitelist based on what
> > a C token is would be better than a blacklist of characters.
> >
> > Could you take a look?
>
> Ping.
>
> Tim.
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r284233 - [clang-move] Add header guard for the new header.

[Tim Northover via cfe-commits]

On 14 October 2016 at 13:33, Tim Northover  wrote:
> On 14 October 2016 at 06:01, Haojian Wu via cfe-commits
>  wrote:
>> +  std::string GuardName(FileName);
>> +  if (IsHeader) {
>> +std::replace(GuardName.begin(), GuardName.end(), '/', '_');
>> +std::replace(GuardName.begin(), GuardName.end(), '.', '_');
>> +std::replace(GuardName.begin(), GuardName.end(), '-', '_');
>
> I think this is causing problems with one of our bots that has an '@'
> in a path it uses. In general it seems like a whitelist based on what
> a C token is would be better than a blacklist of characters.
>
> Could you take a look?

Ping.

Tim.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r284233 - [clang-move] Add header guard for the new header.

[Tim Northover via cfe-commits]

Hi Haojian,

On 14 October 2016 at 06:01, Haojian Wu via cfe-commits
 wrote:
> +  std::string GuardName(FileName);
> +  if (IsHeader) {
> +std::replace(GuardName.begin(), GuardName.end(), '/', '_');
> +std::replace(GuardName.begin(), GuardName.end(), '.', '_');
> +std::replace(GuardName.begin(), GuardName.end(), '-', '_');

I think this is causing problems with one of our bots that has an '@'
in a path it uses. In general it seems like a whitelist based on what
a C token is would be better than a blacklist of characters.

Could you take a look?

Cheers.

Tim.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r284233 - [clang-move] Add header guard for the new header.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Fri Oct 14 08:01:36 2016
New Revision: 284233

URL: http://llvm.org/viewvc/llvm-project?rev=284233=rev
Log:
[clang-move] Add header guard for the new header.

Summary:
The header guard generated by clang-move isn't always a perfect
style, just avoid getting the header included multiple times during
compiling period.

Also, we can use llvm-Header-guard clang-tidy check to correct the guard
automatically.

Reviewers: ioeric

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D25610

Modified:
clang-tools-extra/trunk/clang-move/ClangMove.cpp
clang-tools-extra/trunk/test/clang-move/move-class.cpp
clang-tools-extra/trunk/test/clang-move/move-multiple-classes.cpp
clang-tools-extra/trunk/unittests/clang-move/ClangMoveTests.cpp

Modified: clang-tools-extra/trunk/clang-move/ClangMove.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-move/ClangMove.cpp?rev=284233=284232=284233=diff
==
--- clang-tools-extra/trunk/clang-move/ClangMove.cpp (original)
+++ clang-tools-extra/trunk/clang-move/ClangMove.cpp Fri Oct 14 08:01:36 2016
@@ -249,8 +249,22 @@ std::vector GetNamespaces(c
 clang::tooling::Replacements
 createInsertedReplacements(const std::vector ,
const std::vector ,
-   llvm::StringRef FileName) {
+   llvm::StringRef FileName,
+   bool IsHeader = false) {
   clang::tooling::Replacements InsertedReplacements;
+  std::string GuardName(FileName);
+  if (IsHeader) {
+std::replace(GuardName.begin(), GuardName.end(), '/', '_');
+std::replace(GuardName.begin(), GuardName.end(), '.', '_');
+std::replace(GuardName.begin(), GuardName.end(), '-', '_');
+
+GuardName = StringRef(GuardName).upper();
+std::string HeaderGuard = "#ifndef " + GuardName + "\n";
+HeaderGuard += "#define " + GuardName + "\n";
+clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0,
+   HeaderGuard);
+addOrMergeReplacement(HeaderGuardInclude, );
+  }
 
   // Add #Includes.
   std::string AllIncludesString;
@@ -308,6 +322,12 @@ createInsertedReplacements(const std::ve
 FileName, 0, 0, "} // namespace " + NS + "\n");
 addOrMergeReplacement(InsertedReplacement, );
   }
+
+  if (IsHeader) {
+clang::tooling::Replacement HeaderGuardEnd(FileName, 0, 0,
+   "#endif // " + GuardName + 
"\n");
+addOrMergeReplacement(HeaderGuardEnd, );
+  }
   return InsertedReplacements;
 }
 
@@ -500,7 +520,7 @@ void ClangMoveTool::moveClassDefinitionT
 
   if (!Spec.NewHeader.empty())
 FileToReplacements[Spec.NewHeader] = createInsertedReplacements(
-HeaderIncludes, NewHeaderDecls, Spec.NewHeader);
+HeaderIncludes, NewHeaderDecls, Spec.NewHeader, /*IsHeader=*/true);
   if (!Spec.NewCC.empty())
 FileToReplacements[Spec.NewCC] =
 createInsertedReplacements(CCIncludes, NewCCDecls, Spec.NewCC);

Modified: clang-tools-extra/trunk/test/clang-move/move-class.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-move/move-class.cpp?rev=284233=284232=284233=diff
==
--- clang-tools-extra/trunk/test/clang-move/move-class.cpp (original)
+++ clang-tools-extra/trunk/test/clang-move/move-class.cpp Fri Oct 14 08:01:36 
2016
@@ -21,6 +21,9 @@
 // RUN: FileCheck -input-file=%T/clang-move/src/test.cpp 
-check-prefix=CHECK-OLD-TEST-CPP %s
 // RUN: FileCheck -input-file=%T/clang-move/include/test.h %s 
-implicit-check-not='{{namespace.*}}'
 //
+//
+// CHECK-NEW-TEST-H: #ifndef {{.*}}CLANG_MOVE_NEW_TEST_H
+// CHECK-NEW-TEST-H: #define {{.*}}CLANG_MOVE_NEW_TEST_H
 // CHECK-NEW-TEST-H: namespace a {
 // CHECK-NEW-TEST-H: class Foo {
 // CHECK-NEW-TEST-H: public:
@@ -28,6 +31,7 @@
 // CHECK-NEW-TEST-H:   int f2(int a, int b);
 // CHECK-NEW-TEST-H: };
 // CHECK-NEW-TEST-H: } // namespace a
+// CHECK-NEW-TEST-H: #endif // {{.*}}CLANG_MOVE_NEW_TEST_H
 //
 // CHECK-NEW-TEST-CPP: #include "{{.*}}new_test.h"
 // CHECK-NEW-TEST-CPP: #include "test2.h"

Modified: clang-tools-extra/trunk/test/clang-move/move-multiple-classes.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-move/move-multiple-classes.cpp?rev=284233=284232=284233=diff
==
--- clang-tools-extra/trunk/test/clang-move/move-multiple-classes.cpp (original)
+++ clang-tools-extra/trunk/test/clang-move/move-multiple-classes.cpp Fri Oct 
14 08:01:36 2016
@@ -24,6 +24,8 @@
 // CHECK-OLD-TEST-CPP: }
 // CHECK-OLD-TEST-CPP: } // namespace c
 
+// CHECK-NEW-TEST-H: #ifndef {{.*}}NEW_MULTIPLE_CLASS_TEST_H
+// CHECK-NEW-TEST-H: #define {{.*}}NEW_MULTIPLE_CLASS_TEST_H
 // CHECK-NEW-TEST-H: namespace a {
 //