[PATCH] D131678: hicpp-signed-bitwise - Return location of the operand (and not of the operator beginning)

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun marked an inline comment as done.
vladimir.plyashkun added a comment.

In D131678#3728412 , @njames93 wrote:

> Would you like me to commit it on your behalf again?

Yes, the email is the same - vladimir.plyash...@jetbrains.com


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131678/new/

https://reviews.llvm.org/D131678

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

[PATCH] D131678: hicpp-signed-bitwise - Return location of the operand (and not of the operator beginning)

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 452936.
vladimir.plyashkun added a comment.

- return operand source range in too


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131678/new/

https://reviews.llvm.org/D131678

Files:
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise-integer-literals.cpp
  clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp
@@ -42,7 +42,7 @@
   UResult = SValue & -1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UResult&= 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
 
   UResult = UValue & 1u; // Ok
   UResult = UValue & UValue; // Ok
@@ -63,43 +63,43 @@
 
   // More complex expressions.
   UResult = UValue & (SByte1 + (SByte1 | SByte2));
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   // CHECK-MESSAGES: :[[@LINE-2]]:33: warning: use of a signed integer operand with a binary bitwise operator
 
   // The rest is to demonstrate functionality but all operators are matched equally.
   // Therefore functionality is the same for all binary operations.
   UByte1 = UByte1 | UByte2; // Ok
   UByte1 = UByte1 | SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use of a signed integer operand with a binary bitwise operator
   UByte1|= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
   UByte1|= UByte2; // Ok
 
   UByte1 = UByte1 ^ UByte2; // Ok
   UByte1 = UByte1 ^ SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use of a signed integer operand with a binary bitwise operator
   UByte1^= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
   UByte1^= UByte2; // Ok
 
   UByte1 = UByte1 >> UByte2; // Ok
   UByte1 = UByte1 >> SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   UByte1>>= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UByte1>>= UByte2; // Ok
 
   UByte1 = UByte1 << UByte2; // Ok
   UByte1 = UByte1 << SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   UByte1<<= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UByte1<<= UByte2; // Ok
 
   int SignedInt1 = 1 << 12;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
   int SignedInt2 = 1u << 12;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: use of a signed integer operand with a binary bitwise operator
 }
 
 void f1(unsigned char c) {}
@@ -113,28 +113,28 @@
   UByte1 = ~UByte1; // Ok
   SByte1 = ~UByte1;
   SByte1 = ~SByte1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
   UByte1 = ~SByte1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of 

[PATCH] D131678: hicpp-signed-bitwise - Return location of the operand (and not of the operator beginning)

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 452605.
vladimir.plyashkun added a comment.

- Executed clang-format after changes
- Return operator location (in additional range)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131678/new/

https://reviews.llvm.org/D131678

Files:
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise-integer-literals.cpp
  clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp
@@ -42,7 +42,7 @@
   UResult = SValue & -1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UResult&= 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
 
   UResult = UValue & 1u; // Ok
   UResult = UValue & UValue; // Ok
@@ -63,43 +63,43 @@
 
   // More complex expressions.
   UResult = UValue & (SByte1 + (SByte1 | SByte2));
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   // CHECK-MESSAGES: :[[@LINE-2]]:33: warning: use of a signed integer operand with a binary bitwise operator
 
   // The rest is to demonstrate functionality but all operators are matched equally.
   // Therefore functionality is the same for all binary operations.
   UByte1 = UByte1 | UByte2; // Ok
   UByte1 = UByte1 | SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use of a signed integer operand with a binary bitwise operator
   UByte1|= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
   UByte1|= UByte2; // Ok
 
   UByte1 = UByte1 ^ UByte2; // Ok
   UByte1 = UByte1 ^ SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use of a signed integer operand with a binary bitwise operator
   UByte1^= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
   UByte1^= UByte2; // Ok
 
   UByte1 = UByte1 >> UByte2; // Ok
   UByte1 = UByte1 >> SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   UByte1>>= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UByte1>>= UByte2; // Ok
 
   UByte1 = UByte1 << UByte2; // Ok
   UByte1 = UByte1 << SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   UByte1<<= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UByte1<<= UByte2; // Ok
 
   int SignedInt1 = 1 << 12;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
   int SignedInt2 = 1u << 12;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: use of a signed integer operand with a binary bitwise operator
 }
 
 void f1(unsigned char c) {}
@@ -113,28 +113,28 @@
   UByte1 = ~UByte1; // Ok
   SByte1 = ~UByte1;
   SByte1 = ~SByte1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a unary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a unary bitwise operator
   UByte1 = ~SByte1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a unary bitwise operator
+  //

[PATCH] D131590: Fixed page title for abseil-no-internal-dependencies check documentation

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

In D131590#3716471 , @njames93 wrote:

> In D131590#3716358 , 
> @vladimir.plyashkun wrote:
>
>> Sure it's up to you, it's my github  
>> account , but i don't think it's linked to JetBrains...
>
> Which email address would you like me to use then?

Let's leave "vladimir.plyash...@jetbrains.com"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131590/new/

https://reviews.llvm.org/D131590

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

[PATCH] D131678: hicpp-signed-bitwise - Return location of the operand (and not of the operator beginning)

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added inline comments.



Comment at: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp:100
  "%select{binary|unary}0 bitwise operator")
-  << IsUnary << SignedOperand->getSourceRange();
+  << IsUnary << Location;
 }

njames93 wrote:
> Seems pretty useless to pass the begin location of the Unary/Binary operator 
> here.
> The only source locations that are of interest is the operator location and 
> the whole range of the SignedOperand.
> Based on the issues with jetbrains rendering, I also imagine clangd would 
> have the same issue, We'd likely want to have the diag locations as the 
> SignedOperand begin(like this patch). Then pass the whole source range of the 
> signed operand(like it currently is).
> I'm either way on whether we would need to pass the location of the operator 
> `(BinaryOp|UnaryOp)->getOperatorLoc()`
My initial point was to preserve current behaviour as much as possible, but i 
agree that `(BinaryOp|UnaryOp)->getOperatorLoc()` is much better in this case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131678/new/

https://reviews.llvm.org/D131678

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

[PATCH] D131590: Fixed page title for abseil-no-internal-dependencies check documentation

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

In D131590#3716317 , @njames93 wrote:

> In D131590#3713824 , 
> @vladimir.plyashkun wrote:
>
>> In D131590#3713731 , @njames93 
>> wrote:
>>
>>> Can this be backported as well.
>>
>> I guess someone needs to do it, because i haven't got commit/write access in 
>> LLVM repository.
>
> I can kand this for you and open the backport,
> What is your github username and email(Would I be able to use your jetbrains 
> email?)

Sure it's up to you, it's my [[ https://github.com/VladimirPlyashkun | github  
]]account , but i don't think it's linked to JetBrains...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131590/new/

https://reviews.llvm.org/D131590

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

[PATCH] D131678: hicpp-signed-bitwise - Return location of the operand (and not of the operator beginning)

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

Hi @carlosgalvezp
Yes, sorry, maybe screenshot and example from issue in our bugtracker is not 
the best one. 
You can check it on this sample  from 
clang-tidy tests:
The "hicpp-signed-bitwise" will point to

  URes = UValue & -1;
  ^

instead of -1, which looks confusing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131678/new/

https://reviews.llvm.org/D131678

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

[PATCH] D131678: hicpp-signed-bitwise - Return location of the operand (and not of the operator beginning)

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun created this revision.
vladimir.plyashkun added reviewers: JonasToth, steveire, njames93.
vladimir.plyashkun added a project: clang-tools-extra.
Herald added a subscriber: carlosgalvezp.
Herald added a project: All.
vladimir.plyashkun requested review of this revision.
Herald added a subscriber: cfe-commits.

Hello! 
Currently, the "hicpp/signed-bitwise" check returns the beginning of the 
binary/unary operator as location, which sometimes confuses users in the IDE 
due to incorrect highlighting 
.
 
Yes, the offset from Ranges can be used for this particular check, but i 
suppose better solution is to return begin location of the problematic operand 
instead of operator.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131678

Files:
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise-integer-literals.cpp
  clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/hicpp/signed-bitwise.cpp
@@ -42,7 +42,7 @@
   UResult = SValue & -1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UResult&= 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
 
   UResult = UValue & 1u; // Ok
   UResult = UValue & UValue; // Ok
@@ -63,43 +63,43 @@
 
   // More complex expressions.
   UResult = UValue & (SByte1 + (SByte1 | SByte2));
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   // CHECK-MESSAGES: :[[@LINE-2]]:33: warning: use of a signed integer operand with a binary bitwise operator
 
   // The rest is to demonstrate functionality but all operators are matched equally.
   // Therefore functionality is the same for all binary operations.
   UByte1 = UByte1 | UByte2; // Ok
   UByte1 = UByte1 | SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use of a signed integer operand with a binary bitwise operator
   UByte1|= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
   UByte1|= UByte2; // Ok
 
   UByte1 = UByte1 ^ UByte2; // Ok
   UByte1 = UByte1 ^ SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use of a signed integer operand with a binary bitwise operator
   UByte1^= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
   UByte1^= UByte2; // Ok
 
   UByte1 = UByte1 >> UByte2; // Ok
   UByte1 = UByte1 >> SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   UByte1>>= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UByte1>>= UByte2; // Ok
 
   UByte1 = UByte1 << UByte2; // Ok
   UByte1 = UByte1 << SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use of a signed integer operand with a binary bitwise operator
   UByte1<<= SByte2;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
   UByte1<<= UByte2; // Ok
 
   int SignedInt1 = 1 << 12;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
   int SignedInt2 = 1u << 12;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
+  // CHECK-MESSAGES: 

[PATCH] D131590: Fixed page title for abseil-no-internal-dependencies check documentation

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

In D131590#3713731 , @njames93 wrote:

> Can this be backported as well.

I guess someone needs to do it, because i haven't got commit/write access in 
LLVM repository.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131590/new/

https://reviews.llvm.org/D131590

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

[PATCH] D131590: Fixed page title for abseil-no-internal-dependencies check documentation

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun created this revision.
vladimir.plyashkun added reviewers: aaron.ballman, ivanmurashko, njames93.
vladimir.plyashkun added a project: clang-tools-extra.
Herald added a project: All.
vladimir.plyashkun requested review of this revision.
Herald added a subscriber: cfe-commits.

It seems that documentation for abseil-no-internal-dependencies has invalid 
title.
This can be checked by looking at the actual web-site - 
https://clang.llvm.org/extra/clang-tidy/checks/abseil/no-internal-dependencies.html
There is redundant "subl.. title:: clang-tidy - 
abseil-no-internal-dependencies" paragraph in the beginning.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131590

Files:
  clang-tools-extra/docs/clang-tidy/checks/abseil/no-internal-dependencies.rst


Index: 
clang-tools-extra/docs/clang-tidy/checks/abseil/no-internal-dependencies.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/abseil/no-internal-dependencies.rst
+++ clang-tools-extra/docs/clang-tidy/checks/abseil/no-internal-dependencies.rst
@@ -1,4 +1,4 @@
-subl.. title:: clang-tidy - abseil-no-internal-dependencies
+.. title:: clang-tidy - abseil-no-internal-dependencies
 
 abseil-no-internal-dependencies
 ===


Index: clang-tools-extra/docs/clang-tidy/checks/abseil/no-internal-dependencies.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/abseil/no-internal-dependencies.rst
+++ clang-tools-extra/docs/clang-tidy/checks/abseil/no-internal-dependencies.rst
@@ -1,4 +1,4 @@
-subl.. title:: clang-tidy - abseil-no-internal-dependencies
+.. title:: clang-tidy - abseil-no-internal-dependencies
 
 abseil-no-internal-dependencies
 ===
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68694: [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun marked an inline comment as done.
vladimir.plyashkun added a comment.

In D68694#1711035 , @aaron.ballman 
wrote:

> LGTM aside from a small nit.


Thanks, fixed it!


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68694/new/

https://reviews.llvm.org/D68694



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

[PATCH] D68694: [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 226101.

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68694/new/

https://reviews.llvm.org/D68694

Files:
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst
  clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp

Index: clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s hicpp-signed-bitwise %t -- \
+// RUN:   -config="{CheckOptions: [{key: hicpp-signed-bitwise.IgnorePositiveIntegerLiterals, value: 1 }]}" \
+// RUN: -- -std=c++11
+
+void examples() {
+  unsigned UValue = 40u;
+  unsigned URes;
+  
+  URes = UValue & 1u; //Ok
+  URes = UValue & -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use of a signed integer operand with a binary bitwise operator
+  
+  unsigned URes2 = URes << 1; //Ok
+  
+  int IResult;
+  IResult = 10 & 2; //Ok
+  IResult = 3 << -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  
+  int Int = 30;
+  IResult = Int << 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  IResult = ~0; //Ok
+}
+
+enum EnumConstruction {
+  one = 1,
+  two = 2,
+  test1 = 1 << 12, //Ok
+  test2 = one << two,
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
+  test3 = 1u << 12, //Ok
+};
Index: clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst
+++ clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst
@@ -7,3 +7,11 @@
 undefined or implementation defined behaviour.
 
 The according rule is defined in the `High Integrity C++ Standard, Section 5.6.1 `_.
+
+Options
+---
+
+.. option:: IgnorePositiveIntegerLiterals
+
+   If this option is set to `true`, the check will not warn on bitwise operations with positive integer literals, e.g. `~0`, `2 << 1`, etc. 
+   Default value is `false`.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -169,6 +169,10 @@
   ` check.
 
   Rewrites function signatures to use a trailing return type.
+  
+- The :doc:`hicpp-signed-bitwise
+  ` now supports ``IgnorePositiveIntegerLiterals``
+  option.
 
 Improvements to include-fixer
 -
Index: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
===
--- clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
+++ clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
@@ -22,10 +22,13 @@
 /// http://clang.llvm.org/extra/clang-tidy/checks/hicpp-signed-bitwise.html
 class SignedBitwiseCheck : public ClangTidyCheck {
 public:
-  SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context)
-  : ClangTidyCheck(Name, Context) {}
+  SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
+
+private:
+  bool IgnorePositiveIntegerLiterals;
 };
 
 } // namespace hicpp
Index: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
===
--- clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
+++ clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
@@ -17,9 +17,24 @@
 namespace tidy {
 namespace hicpp {
 
+SignedBitwiseCheck::SignedBitwiseCheck(StringRef Name,
+   ClangTidyContext *Context)
+: ClangTidyCheck(Name, Context),
+  IgnorePositiveIntegerLiterals(
+  Options.get("IgnorePositiveIntegerLiterals", false)) {}
+
+void SignedBitwiseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnorePositiveIntegerLiterals",
+IgnorePositiveIntegerLiterals);
+}
+
 void SignedBitwiseCheck::registerMatchers(MatchFinder *Finder) {
   const auto SignedIntegerOperand =
-  expr(ignoringImpCasts(hasType(isSignedInteger(.bind("signed-operand");
+  (IgnorePositiveIntegerLiterals
+   ? expr(ignoringImpCasts(hasType(isSignedInteger())),
+  unless(intege

[PATCH] D68694: [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

In D68694#1706204 , @JonasToth wrote:

> > Do you know who is responsible for it? Because i haven't worked with 
> > documentation before and don't know what i need to do to update it.
>
> The files reside in `clang-tools-extra/docs/ReleaseNotes.rst` and 
> `clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst`.
>
> You can check other files for reference, e.g. `readability-magic-numbers.rst`.
>  In the Release Notes you can add one entry at the end of the clang-tidy 
> section.


Thanks! I've fixed documentation.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68694/new/

https://reviews.llvm.org/D68694



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

[PATCH] D68694: [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 224976.

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68694/new/

https://reviews.llvm.org/D68694

Files:
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst
  clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp

Index: clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s hicpp-signed-bitwise %t -- \
+// RUN:   -config="{CheckOptions: [{key: hicpp-signed-bitwise.IgnorePositiveIntegerLiterals, value: 1 }]}" \
+// RUN: -- -std=c++11
+
+void examples() {
+  unsigned UValue = 40u;
+  unsigned URes;
+  
+  URes = UValue & 1u; //Ok
+  URes = UValue & -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use of a signed integer operand with a binary bitwise operator
+  
+  unsigned URes2 = URes << 1; //Ok
+  
+  int IResult;
+  IResult = 10 & 2; //Ok
+  IResult = 3 << -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  
+  int Int = 30;
+  IResult = Int << 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  IResult = ~0; //Ok
+}
+
+enum EnumConstruction {
+  one = 1,
+  two = 2,
+  test1 = 1 << 12, //Ok
+  test2 = one << two,
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
+  test3 = 1u << 12, //Ok
+};
Index: clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst
+++ clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst
@@ -7,3 +7,11 @@
 undefined or implementation defined behaviour.
 
 The according rule is defined in the `High Integrity C++ Standard, Section 5.6.1 `_.
+
+Options
+---
+
+.. option:: IgnorePositiveIntegerLiterals
+
+   If this option is set to `true`, the check will not warn on bitwise operations with positive integer literals, e.g. `~0`, `2 << 1`, etc. 
+   Default value is `false`.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -169,6 +169,10 @@
   ` check.
 
   Rewrites function signatures to use a trailing return type.
+  
+- The :doc:`hicpp-signed-bitwise
+  ` now supports `IgnorePositiveIntegerLiterals`
+  option.
 
 Improvements to include-fixer
 -
Index: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
===
--- clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
+++ clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
@@ -22,10 +22,13 @@
 /// http://clang.llvm.org/extra/clang-tidy/checks/hicpp-signed-bitwise.html
 class SignedBitwiseCheck : public ClangTidyCheck {
 public:
-  SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context)
-  : ClangTidyCheck(Name, Context) {}
+  SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
+
+private:
+  bool IgnorePositiveIntegerLiterals;
 };
 
 } // namespace hicpp
Index: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
===
--- clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
+++ clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
@@ -17,9 +17,24 @@
 namespace tidy {
 namespace hicpp {
 
+SignedBitwiseCheck::SignedBitwiseCheck(StringRef Name,
+   ClangTidyContext *Context)
+: ClangTidyCheck(Name, Context),
+  IgnorePositiveIntegerLiterals(
+  Options.get("IgnorePositiveIntegerLiterals", false)) {}
+
+void SignedBitwiseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnorePositiveIntegerLiterals",
+IgnorePositiveIntegerLiterals);
+}
+
 void SignedBitwiseCheck::registerMatchers(MatchFinder *Finder) {
   const auto SignedIntegerOperand =
-  expr(ignoringImpCasts(hasType(isSignedInteger(.bind("signed-operand");
+  (IgnorePositiveIntegerLiterals
+   ? expr(ignoringImpCasts(hasType(isSignedInteger())),
+  unless(integerL

[PATCH] D68694: [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

In D68694#1705668 , @JonasToth wrote:

> The new switch needs documentation as well, and maybe even a note in the 
> release notes (as it is publicly discussed as issue?).


Do you know who is responsible for it? Because i haven't worked with 
documentation before and don't know what i need to do to update it.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68694/new/

https://reviews.llvm.org/D68694



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

[PATCH] D68694: [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 224597.

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68694/new/

https://reviews.llvm.org/D68694

Files:
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
  clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp


Index: 
clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s hicpp-signed-bitwise %t -- \
+// RUN:   -config="{CheckOptions: [{key: 
hicpp-signed-bitwise.IgnorePositiveIntegerLiterals, value: 1 }]}" \
+// RUN: -- -std=c++11
+
+void examples() {
+  unsigned UValue = 40u;
+  unsigned URes;
+  
+  URes = UValue & 1u; //Ok
+  URes = UValue & -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use of a signed integer operand 
with a binary bitwise operator
+  
+  unsigned URes2 = URes << 1; //Ok
+  
+  int IResult;
+  IResult = 10 & 2; //Ok
+  IResult = 3 << -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand 
with a binary bitwise operator
+  
+  int Int = 30;
+  IResult = Int << 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand 
with a binary bitwise operator
+  IResult = ~0; //Ok
+}
+
+enum EnumConstruction {
+  one = 1,
+  two = 2,
+  test1 = 1 << 12, //Ok
+  test2 = one << two,
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use of a signed integer operand 
with a binary bitwise operator
+  test3 = 1u << 12, //Ok
+};
Index: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
===
--- clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
+++ clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
@@ -22,10 +22,13 @@
 /// http://clang.llvm.org/extra/clang-tidy/checks/hicpp-signed-bitwise.html
 class SignedBitwiseCheck : public ClangTidyCheck {
 public:
-  SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context)
-  : ClangTidyCheck(Name, Context) {}
+  SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
+
+private:
+  bool IgnorePositiveIntegerLiterals;
 };
 
 } // namespace hicpp
Index: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
===
--- clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
+++ clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
@@ -17,9 +17,24 @@
 namespace tidy {
 namespace hicpp {
 
+SignedBitwiseCheck::SignedBitwiseCheck(StringRef Name,
+   ClangTidyContext *Context)
+: ClangTidyCheck(Name, Context),
+  IgnorePositiveIntegerLiterals(
+  Options.get("IgnorePositiveIntegerLiterals", false)) {}
+
+void SignedBitwiseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnorePositiveIntegerLiterals",
+IgnorePositiveIntegerLiterals);
+}
+
 void SignedBitwiseCheck::registerMatchers(MatchFinder *Finder) {
   const auto SignedIntegerOperand =
-  
expr(ignoringImpCasts(hasType(isSignedInteger(.bind("signed-operand");
+  (IgnorePositiveIntegerLiterals
+   ? expr(ignoringImpCasts(hasType(isSignedInteger())),
+  unless(integerLiteral()))
+   : expr(ignoringImpCasts(hasType(isSignedInteger()
+  .bind("signed-operand");
 
   // The standard [bitmask.types] allows some integral types to be implemented
   // as signed types. Exclude these types from diagnosing for bitwise or(|) and


Index: clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s hicpp-signed-bitwise %t -- \
+// RUN:   -config="{CheckOptions: [{key: hicpp-signed-bitwise.IgnorePositiveIntegerLiterals, value: 1 }]}" \
+// RUN: -- -std=c++11
+
+void examples() {
+  unsigned UValue = 40u;
+  unsigned URes;
+  
+  URes = UValue & 1u; //Ok
+  URes = UValue & -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use of a signed integer operand with a binary bitwise operator
+  
+  unsigned URes2 = URes << 1; //Ok
+  
+  int IResult;
+  IResult = 10 & 2; //Ok
+  IResult = 3 << -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  
+  int Int = 30;
+  IResult = Int << 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer o

[PATCH] D68694: [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun marked 2 inline comments as done.
vladimir.plyashkun added inline comments.



Comment at: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp:23-24
+: ClangTidyCheck(Name, Context),
+  IgnorePositiveIntegerLiterals(
+  Options.getLocalOrGlobal("IgnorePositiveIntegerLiterals", false)) {}
+

lebedev.ri wrote:
> vladimir.plyashkun wrote:
> > lebedev.ri wrote:
> > > i'm not sure this should look for a global option with such name?
> > I think that this method is common and used in so many inspections. 
> > For example this [[ 
> > https://clang.llvm.org/extra/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.html
> >   | check ]] also have option called `IgnoreMacros` which is retrieved in 
> > same way (by calling getLocalOrGlobal method)
> I'm very specifically discriminating against 
> `"IgnorePositiveIntegerLiterals"` here.
> I know `getLocalOrGlobal()` is widely used, because in those places the same 
> flag
> is used in multiple modules with same meaning.
> Is that the case here?
Ok, i agree. Fixed.



Comment at: 
clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp:19
+  int Int = 30;
+  IResult = Int << 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand 
with a binary bitwise operator

JonasToth wrote:
> Could you please add `URes << 1` as well? I believe that was problematic in 
> the stack-overflow-question, wasn't it?
Yes, sure.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68694/new/

https://reviews.llvm.org/D68694



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

[PATCH] D68694: [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun marked an inline comment as done.
vladimir.plyashkun added inline comments.



Comment at: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp:23-24
+: ClangTidyCheck(Name, Context),
+  IgnorePositiveIntegerLiterals(
+  Options.getLocalOrGlobal("IgnorePositiveIntegerLiterals", false)) {}
+

lebedev.ri wrote:
> i'm not sure this should look for a global option with such name?
I think that this method is common and used in so many inspections. 
For example this [[ 
https://clang.llvm.org/extra/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.html
  | check ]] also have option called `IgnoreMacros` which is retrieved in same 
way (by calling getLocalOrGlobal method)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68694/new/

https://reviews.llvm.org/D68694



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

[PATCH] D68694: [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

@aaron.ballman
@JonasToth
Thanks, i agree too! I've prepared new revision with additional option to 
preserve old inspection behaviour.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68694/new/

https://reviews.llvm.org/D68694



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

[PATCH] D68694: [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 224557.
vladimir.plyashkun added a comment.

Provide additional option to preserve current inspection behaviour


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68694/new/

https://reviews.llvm.org/D68694

Files:
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
  clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp


Index: 
clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
@@ -0,0 +1,31 @@
+// RUN: %check_clang_tidy %s hicpp-signed-bitwise %t -- \
+// RUN:   -config="{CheckOptions: [{key: 
hicpp-signed-bitwise.IgnorePositiveIntegerLiterals, value: 1 }]}" \
+// RUN: -- -std=c++11
+
+void examples() {
+  unsigned UValue = 40u;
+  unsigned URes;
+  
+  URes = UValue & 1u; //Ok
+  URes = UValue & -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use of a signed integer operand 
with a binary bitwise operator
+  
+  int IResult;
+  IResult = 10 & 2; //Ok
+  IResult = 3 << -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand 
with a binary bitwise operator
+  
+  int Int = 30;
+  IResult = Int << 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand 
with a binary bitwise operator
+  IResult = ~0; //Ok
+}
+
+enum EnumConstruction {
+  one = 1,
+  two = 2,
+  test1 = 1 << 12, //Ok
+  test2 = one << two,
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use of a signed integer operand 
with a binary bitwise operator
+  test3 = 1u << 12, //Ok
+};
Index: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
===
--- clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
+++ clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
@@ -22,10 +22,13 @@
 /// http://clang.llvm.org/extra/clang-tidy/checks/hicpp-signed-bitwise.html
 class SignedBitwiseCheck : public ClangTidyCheck {
 public:
-  SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context)
-  : ClangTidyCheck(Name, Context) {}
+  SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
+
+private:
+  bool IgnorePositiveIntegerLiterals;
 };
 
 } // namespace hicpp
Index: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
===
--- clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
+++ clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
@@ -17,9 +17,24 @@
 namespace tidy {
 namespace hicpp {
 
+SignedBitwiseCheck::SignedBitwiseCheck(StringRef Name,
+   ClangTidyContext *Context)
+: ClangTidyCheck(Name, Context),
+  IgnorePositiveIntegerLiterals(
+  Options.getLocalOrGlobal("IgnorePositiveIntegerLiterals", false)) {}
+
+void SignedBitwiseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnorePositiveIntegerLiterals",
+IgnorePositiveIntegerLiterals);
+}
+
 void SignedBitwiseCheck::registerMatchers(MatchFinder *Finder) {
   const auto SignedIntegerOperand =
-  
expr(ignoringImpCasts(hasType(isSignedInteger(.bind("signed-operand");
+  (IgnorePositiveIntegerLiterals
+   ? expr(ignoringImpCasts(hasType(isSignedInteger())),
+  unless(integerLiteral()))
+   : expr(ignoringImpCasts(hasType(isSignedInteger()
+  .bind("signed-operand");
 
   // The standard [bitmask.types] allows some integral types to be implemented
   // as signed types. Exclude these types from diagnosing for bitwise or(|) and


Index: clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
@@ -0,0 +1,31 @@
+// RUN: %check_clang_tidy %s hicpp-signed-bitwise %t -- \
+// RUN:   -config="{CheckOptions: [{key: hicpp-signed-bitwise.IgnorePositiveIntegerLiterals, value: 1 }]}" \
+// RUN: -- -std=c++11
+
+void examples() {
+  unsigned UValue = 40u;
+  unsigned URes;
+  
+  URes = UValue & 1u; //Ok
+  URes = UValue & -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use of a signed integer operand with a binary bitwise operator
+  
+  int IResult;
+  IResult = 10 & 2; //Ok
+  IResult = 3 << -1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use of a signed integer operand with a binary bitwise operator
+  
+  int Int = 30;
+  IResult = Int << 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:

[PATCH] D68694: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun created this revision.
vladimir.plyashkun added a reviewer: JonasToth.
vladimir.plyashkun added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, mgehre.
Herald added a project: clang.

Hi! Clang-Tidy has been integrated in CLion some time ago, but a lot of users 
started to complain on `hicpp-signed-bitwise` inspection as it produces a lot 
of noise, especially for positive integer literals.

There are already some issues (with discussions) in LLVM tracker with same 
problem:
https://bugs.llvm.org/show_bug.cgi?id=36961
https://bugs.llvm.org/show_bug.cgi?id=43606

In my opinion this check should be disabled in case of integer literals, since 
there are a lot of existing code (even in system libraries) where user can do 
nothing, e.g.:

  #include 
  
  int main() {
  open("file", O_RDONLY | O_NOCTTY); // <-- warning here
  }


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D68694

Files:
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
  clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise.cpp


Index: clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise.cpp
===
--- clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise.cpp
+++ clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise.cpp
@@ -96,10 +96,8 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand 
with a binary bitwise operator
   UByte1<<= UByte2; // Ok
 
-  int SignedInt1 = 1 << 12;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand 
with a binary bitwise operator
-  int SignedInt2 = 1u << 12;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand 
with a binary bitwise operator
+  int SignedInt1 = 1 << 12; // Ok
+  int SignedInt2 = 1u << 12; // Ok
 }
 
 void f1(unsigned char c) {}
@@ -157,8 +155,7 @@
   r = -1 >> -i;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand 
with a binary bitwise operator
 
-  r = ~0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand 
with a unary bitwise operator
+  r = ~0; // Ok
   r = ~0u; // Ok
   k = ~k;  // Ok
 
@@ -231,10 +228,8 @@
 enum EnumConstruction {
   one = 1,
   two = 2,
-  test1 = 1 << 12,
-  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand 
with a binary bitwise operator
+  test1 = 1 << 12, // Ok
   test2 = one << two,
   // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand 
with a binary bitwise operator
-  test3 = 1u << 12,
-  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand 
with a binary bitwise operator
+  test3 = 1u << 12, // Ok
 };
Index: clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
===
--- clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
+++ clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
@@ -19,7 +19,9 @@
 
 void SignedBitwiseCheck::registerMatchers(MatchFinder *Finder) {
   const auto SignedIntegerOperand =
-  
expr(ignoringImpCasts(hasType(isSignedInteger(.bind("signed-operand");
+  expr(ignoringImpCasts(hasType(isSignedInteger())),
+   unless(integerLiteral()))
+  .bind("signed-operand");
 
   // The standard [bitmask.types] allows some integral types to be implemented
   // as signed types. Exclude these types from diagnosing for bitwise or(|) and


Index: clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise.cpp
===
--- clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise.cpp
+++ clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise.cpp
@@ -96,10 +96,8 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of a signed integer operand with a binary bitwise operator
   UByte1<<= UByte2; // Ok
 
-  int SignedInt1 = 1 << 12;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
-  int SignedInt2 = 1u << 12;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use of a signed integer operand with a binary bitwise operator
+  int SignedInt1 = 1 << 12; // Ok
+  int SignedInt2 = 1u << 12; // Ok
 }
 
 void f1(unsigned char c) {}
@@ -157,8 +155,7 @@
   r = -1 >> -i;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a binary bitwise operator
 
-  r = ~0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use of a signed integer operand with a unary bitwise operator
+  r = ~0; // Ok
   r = ~0u; // Ok
   k = ~k;  // Ok
 
@@ -231,10 +228,8 @@
 enum EnumConstruction {
   one = 1,
   two = 2,
-  test1 = 1 << 12,
-  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
+  test1 = 1 << 12, // Ok
   test2 = one << two,
   // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use of a signed integer operand with a binary bitwise operator
-  test3 = 1u

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 130891.
vladimir.plyashkun added a comment.

Thanks, now all should be fine!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/tool/ClangTidyMain.cpp
  test/clang-tidy/Inputs/vfsoverlay/actual_header.h
  test/clang-tidy/Inputs/vfsoverlay/vfsoverlay.yaml
  test/clang-tidy/vfsoverlay.cpp

Index: test/clang-tidy/vfsoverlay.cpp
===
--- /dev/null
+++ test/clang-tidy/vfsoverlay.cpp
@@ -0,0 +1,8 @@
+// RUN: sed -e "s:INPUT_DIR:%S/Inputs/vfsoverlay:g" -e "s:OUT_DIR:%t:g" %S/Inputs/vfsoverlay/vfsoverlay.yaml > %t.yaml
+// RUN: clang-tidy %s -checks='-*,modernize-use-nullptr' -vfsoverlay %t.yaml -- -I %t | FileCheck %s
+// REQUIRES: shell
+
+#include "not_real.h"
+
+X *ptr = 0;
+// CHECK: warning: use nullptr [modernize-use-nullptr]
Index: test/clang-tidy/Inputs/vfsoverlay/vfsoverlay.yaml
===
--- /dev/null
+++ test/clang-tidy/Inputs/vfsoverlay/vfsoverlay.yaml
@@ -0,0 +1,12 @@
+{
+  'version': 0,
+  'roots': [
+{ 'name': 'OUT_DIR', 'type': 'directory',
+  'contents': [
+{ 'name': 'not_real.h', 'type': 'file',
+  'external-contents': 'INPUT_DIR/actual_header.h'
+}
+  ]
+}
+  ]
+}
Index: test/clang-tidy/Inputs/vfsoverlay/actual_header.h
===
--- /dev/null
+++ test/clang-tidy/Inputs/vfsoverlay/actual_header.h
@@ -0,0 +1 @@
+struct X {};
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -209,6 +209,12 @@
cl::init(false),
cl::cat(ClangTidyCategory));
 
+static cl::opt VfsOverlay("vfsoverlay", cl::desc(R"(
+Overlay the virtual filesystem described by file over the real file system.
+)"),
+   cl::value_desc("filename"),
+   cl::cat(ClangTidyCategory));
+
 namespace clang {
 namespace tidy {
 
@@ -330,6 +336,29 @@
 OverrideOptions);
 }
 
+llvm::IntrusiveRefCntPtr
+getVfsOverlayFromFile(const std::string &OverlayFile) {
+  llvm::IntrusiveRefCntPtr OverlayFS(
+  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+  llvm::ErrorOr> Buffer =
+  OverlayFS->getBufferForFile(OverlayFile);
+  if (!Buffer) {
+llvm::errs() << "Can't load virtual filesystem overlay file: '"
+ << OverlayFile << "' " << Buffer.getError().message() << ".\n";
+return nullptr;
+  }
+
+  IntrusiveRefCntPtr FS = vfs::getVFSFromYAML(
+  std::move(Buffer.get()), /*DiagHandler*/ nullptr, OverlayFile);
+  if (!FS) {
+llvm::errs() << "Error: invalid virtual filesystem overlay file '"
+ << OverlayFile << "'.\n";
+return nullptr;
+  }
+  OverlayFS->pushOverlay(FS);
+  return OverlayFS;
+}
+
 static int clangTidyMain(int argc, const char **argv) {
   CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
 cl::ZeroOrMore);
@@ -401,6 +430,11 @@
 llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
 return 0;
   }
+  llvm::IntrusiveRefCntPtr BaseFS(
+  VfsOverlay.empty() ? vfs::getRealFileSystem()
+ : getVfsOverlayFromFile(VfsOverlay));
+  if (!BaseFS)
+return 1;
 
   ProfileData Profile;
 
@@ -410,7 +444,7 @@
 
   ClangTidyContext Context(std::move(OwningOptionsProvider));
   runClangTidy(Context, OptionsParser.getCompilations(), PathList,
-   EnableCheckProfile ? &Profile : nullptr);
+   BaseFS, EnableCheckProfile ? &Profile : nullptr);
   ArrayRef Errors = Context.getErrors();
   bool FoundErrors =
   std::find_if(Errors.begin(), Errors.end(), [](const ClangTidyError &E) {
@@ -422,7 +456,7 @@
   unsigned WErrorCount = 0;
 
   // -fix-errors implies -fix.
-  handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount);
+  handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount, BaseFS);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
 std::error_code EC;
Index: clang-tidy/ClangTidy.h
===
--- clang-tidy/ClangTidy.h
+++ clang-tidy/ClangTidy.h
@@ -227,6 +227,7 @@
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
   const tooling::CompilationDatabase &Compilations,
   ArrayRef InputFiles,
+  llvm::IntrusiveRefCntPtr BaseFS,
   ProfileData *Profile = nullptr);
 
 // FIXME: This interface will need to be significantly extended to be useful.
@@ -236,7 +237,8 @@
 /// Errors containing fixes are automatically applied and reformatted. If no
 /// clang-format configurati

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

In https://reviews.llvm.org/D41535#981928, @ilya-biryukov wrote:

> This looks good, but we should add a test.
>  Should've noticed that before, sorry for the slowing this down a bit more. 
> After the test is there, I'm happy to commit this change for you.


IIUC, it will be little bit difficult to test it, because whole logic placed in 
the `ClangTidyMain`. 
All existing clang-tidy unit tests use direct calls of `ToolInvocation` which 
is doesn't know about `vfsoverlay` options.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535



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

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 130589.
vladimir.plyashkun added a comment.

Some more nits were fixed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/tool/ClangTidyMain.cpp

Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -209,6 +209,12 @@
cl::init(false),
cl::cat(ClangTidyCategory));
 
+static cl::opt VfsOverlay("vfsoverlay", cl::desc(R"(
+Overlay the virtual filesystem described by file over the real file system.
+)"),
+   cl::value_desc("filename"),
+   cl::cat(ClangTidyCategory));
+
 namespace clang {
 namespace tidy {
 
@@ -330,6 +336,29 @@
 OverrideOptions);
 }
 
+llvm::IntrusiveRefCntPtr
+getVfsOverlayFromFile(const std::string &OverlayFile) {
+  llvm::IntrusiveRefCntPtr OverlayFS(
+  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+  llvm::ErrorOr> Buffer =
+  OverlayFS->getBufferForFile(OverlayFile);
+  if (!Buffer) {
+llvm::errs() << "Can't load virtual filesystem overlay file: '"
+ << OverlayFile << "' " << Buffer.getError().message() << ".\n";
+return nullptr;
+  }
+
+  IntrusiveRefCntPtr FS = vfs::getVFSFromYAML(
+  std::move(Buffer.get()), /*DiagHandler*/ nullptr, OverlayFile);
+  if (!FS) {
+llvm::errs() << "Error: invalid virtual filesystem overlay file '"
+ << OverlayFile << "'.\n";
+return nullptr;
+  }
+  OverlayFS->pushOverlay(FS);
+  return OverlayFS;
+}
+
 static int clangTidyMain(int argc, const char **argv) {
   CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
 cl::ZeroOrMore);
@@ -401,6 +430,11 @@
 llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
 return 0;
   }
+  llvm::IntrusiveRefCntPtr BaseFS(
+  VfsOverlay.empty() ? vfs::getRealFileSystem()
+ : getVfsOverlayFromFile(VfsOverlay));
+  if (!BaseFS)
+return 1;
 
   ProfileData Profile;
 
@@ -410,7 +444,7 @@
 
   ClangTidyContext Context(std::move(OwningOptionsProvider));
   runClangTidy(Context, OptionsParser.getCompilations(), PathList,
-   EnableCheckProfile ? &Profile : nullptr);
+   BaseFS, EnableCheckProfile ? &Profile : nullptr);
   ArrayRef Errors = Context.getErrors();
   bool FoundErrors =
   std::find_if(Errors.begin(), Errors.end(), [](const ClangTidyError &E) {
@@ -422,7 +456,7 @@
   unsigned WErrorCount = 0;
 
   // -fix-errors implies -fix.
-  handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount);
+  handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount, BaseFS);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
 std::error_code EC;
Index: clang-tidy/ClangTidy.h
===
--- clang-tidy/ClangTidy.h
+++ clang-tidy/ClangTidy.h
@@ -227,6 +227,7 @@
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
   const tooling::CompilationDatabase &Compilations,
   ArrayRef InputFiles,
+  llvm::IntrusiveRefCntPtr BaseFS,
   ProfileData *Profile = nullptr);
 
 // FIXME: This interface will need to be significantly extended to be useful.
@@ -236,7 +237,8 @@
 /// Errors containing fixes are automatically applied and reformatted. If no
 /// clang-format configuration file is found, the given \P FormatStyle is used.
 void handleErrors(ClangTidyContext &Context, bool Fix,
-  unsigned &WarningsAsErrorsCount);
+  unsigned &WarningsAsErrorsCount,
+  llvm::IntrusiveRefCntPtr BaseFS);
 
 /// \brief Serializes replacements into YAML and writes them to the specified
 /// output stream.
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -89,8 +89,10 @@
 
 class ErrorReporter {
 public:
-  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes)
-  : Files(FileSystemOptions()), DiagOpts(new DiagnosticOptions()),
+  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
+llvm::IntrusiveRefCntPtr BaseFS)
+  : Files(FileSystemOptions(), BaseFS), 
+DiagOpts(new DiagnosticOptions()),
 DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
 Diags(IntrusiveRefCntPtr(new DiagnosticIDs), &*DiagOpts,
   DiagPrinter),
@@ -474,8 +476,11 @@
 
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
   const CompilationDatabase &Compilations,
-  ArrayRef InputFiles, ProfileData *Profile) {
-  ClangTool Tool(Compilations, InputFil

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 130578.
vladimir.plyashkun added a comment.

Fixed code review remarks.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/tool/ClangTidyMain.cpp

Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -209,6 +209,12 @@
cl::init(false),
cl::cat(ClangTidyCategory));
 
+static cl::opt VfsOverlay("vfsoverlay", cl::desc(R"(
+Overlay the virtual filesystem described by file over the real file system.
+)"),
+   cl::value_desc("filename"),
+   cl::cat(ClangTidyCategory));
+
 namespace clang {
 namespace tidy {
 
@@ -330,6 +336,29 @@
 OverrideOptions);
 }
 
+llvm::IntrusiveRefCntPtr
+getVfsOverlayFromFile(const std::string &OverlayFile) {
+  llvm::IntrusiveRefCntPtr OverlayFS(
+  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+  llvm::ErrorOr> Buffer =
+  OverlayFS->getBufferForFile(OverlayFile);
+  if (!Buffer) {
+llvm::errs() << "Error: virtual filesystem overlay file '" << OverlayFile
+ << "' not found.\n";
+return nullptr;
+  }
+
+  IntrusiveRefCntPtr FS = vfs::getVFSFromYAML(
+  std::move(Buffer.get()), /*DiagHandler*/ nullptr, OverlayFile);
+  if (!FS.get()) {
+llvm::errs() << "Error: invalid virtual filesystem overlay file '"
+ << OverlayFile << "'.\n";
+return nullptr;
+  }
+  OverlayFS->pushOverlay(FS);
+  return OverlayFS;
+}
+
 static int clangTidyMain(int argc, const char **argv) {
   CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
 cl::ZeroOrMore);
@@ -401,6 +430,12 @@
 llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
 return 0;
   }
+  llvm::IntrusiveRefCntPtr BaseFS(
+  VfsOverlay.empty() ? vfs::getRealFileSystem()
+ : getVfsOverlayFromFile(VfsOverlay));
+  if (!BaseFS) {
+return 1;
+  }
 
   ProfileData Profile;
 
@@ -410,7 +445,7 @@
 
   ClangTidyContext Context(std::move(OwningOptionsProvider));
   runClangTidy(Context, OptionsParser.getCompilations(), PathList,
-   EnableCheckProfile ? &Profile : nullptr);
+   BaseFS, EnableCheckProfile ? &Profile : nullptr);
   ArrayRef Errors = Context.getErrors();
   bool FoundErrors =
   std::find_if(Errors.begin(), Errors.end(), [](const ClangTidyError &E) {
@@ -422,7 +457,7 @@
   unsigned WErrorCount = 0;
 
   // -fix-errors implies -fix.
-  handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount);
+  handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount, BaseFS);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
 std::error_code EC;
Index: clang-tidy/ClangTidy.h
===
--- clang-tidy/ClangTidy.h
+++ clang-tidy/ClangTidy.h
@@ -227,6 +227,7 @@
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
   const tooling::CompilationDatabase &Compilations,
   ArrayRef InputFiles,
+  llvm::IntrusiveRefCntPtr BaseFS,
   ProfileData *Profile = nullptr);
 
 // FIXME: This interface will need to be significantly extended to be useful.
@@ -236,7 +237,8 @@
 /// Errors containing fixes are automatically applied and reformatted. If no
 /// clang-format configuration file is found, the given \P FormatStyle is used.
 void handleErrors(ClangTidyContext &Context, bool Fix,
-  unsigned &WarningsAsErrorsCount);
+  unsigned &WarningsAsErrorsCount,
+  llvm::IntrusiveRefCntPtr BaseFS);
 
 /// \brief Serializes replacements into YAML and writes them to the specified
 /// output stream.
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -89,8 +89,10 @@
 
 class ErrorReporter {
 public:
-  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes)
-  : Files(FileSystemOptions()), DiagOpts(new DiagnosticOptions()),
+  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
+llvm::IntrusiveRefCntPtr BaseFS)
+  : Files(FileSystemOptions(), BaseFS), 
+DiagOpts(new DiagnosticOptions()),
 DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
 Diags(IntrusiveRefCntPtr(new DiagnosticIDs), &*DiagOpts,
   DiagPrinter),
@@ -474,8 +476,11 @@
 
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
   const CompilationDatabase &Compilations,
-  ArrayRef InputFiles, ProfileData *Profile) {
-  ClangTool Tool(Compilations, InputFiles);
+  

[PATCH] D41947: Provide default virtual filesystem argument to ClangTool constructor

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 130573.
vladimir.plyashkun added a comment.

Fixed comment


Repository:
  rC Clang

https://reviews.llvm.org/D41947

Files:
  include/clang/Tooling/Tooling.h
  lib/Tooling/Tooling.cpp
  unittests/Tooling/ToolingTest.cpp


Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -402,6 +402,24 @@
   EXPECT_FALSE(Found);
 }
 
+TEST(ClangToolTest, BaseVirtualFileSystemUsage) {
+  FixedCompilationDatabase Compilations("/", std::vector());
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+
+  InMemoryFileSystem->addFile(
+  "a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+
+  ClangTool Tool(Compilations, std::vector(1, "a.cpp"),
+ std::make_shared(), 
OverlayFileSystem);
+  std::unique_ptr Action(
+  newFrontendActionFactory());
+  EXPECT_EQ(0, Tool.run(Action.get()));
+}
+
 // Check getClangStripDependencyFileAdjuster doesn't strip args after -MD/-MMD.
 TEST(ClangToolTest, StripDependencyFileAdjuster) {
   FixedCompilationDatabase Compilations("/", {"-MD", "-c", "-MMD", "-w"});
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -328,10 +328,11 @@
 
 ClangTool::ClangTool(const CompilationDatabase &Compilations,
  ArrayRef SourcePaths,
- std::shared_ptr PCHContainerOps)
+ std::shared_ptr PCHContainerOps,
+ IntrusiveRefCntPtr BaseFS)
 : Compilations(Compilations), SourcePaths(SourcePaths),
   PCHContainerOps(std::move(PCHContainerOps)),
-  OverlayFileSystem(new vfs::OverlayFileSystem(vfs::getRealFileSystem())),
+  OverlayFileSystem(new vfs::OverlayFileSystem(BaseFS)),
   InMemoryFileSystem(new vfs::InMemoryFileSystem),
   Files(new FileManager(FileSystemOptions(), OverlayFileSystem)),
   DiagConsumer(nullptr) {
Index: include/clang/Tooling/Tooling.h
===
--- include/clang/Tooling/Tooling.h
+++ include/clang/Tooling/Tooling.h
@@ -296,10 +296,14 @@
   ///not found in Compilations, it is skipped.
   /// \param PCHContainerOps The PCHContainerOperations for loading and 
creating
   /// clang modules.
+  /// \param BaseFS VFS used for all underlying file accesses 
+  ///when running the tool.
   ClangTool(const CompilationDatabase &Compilations,
 ArrayRef SourcePaths,
 std::shared_ptr PCHContainerOps =
-std::make_shared());
+std::make_shared(),
+IntrusiveRefCntPtr BaseFS = 
+vfs::getRealFileSystem());
 
   ~ClangTool();
 


Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -402,6 +402,24 @@
   EXPECT_FALSE(Found);
 }
 
+TEST(ClangToolTest, BaseVirtualFileSystemUsage) {
+  FixedCompilationDatabase Compilations("/", std::vector());
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+
+  InMemoryFileSystem->addFile(
+  "a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+
+  ClangTool Tool(Compilations, std::vector(1, "a.cpp"),
+ std::make_shared(), OverlayFileSystem);
+  std::unique_ptr Action(
+  newFrontendActionFactory());
+  EXPECT_EQ(0, Tool.run(Action.get()));
+}
+
 // Check getClangStripDependencyFileAdjuster doesn't strip args after -MD/-MMD.
 TEST(ClangToolTest, StripDependencyFileAdjuster) {
   FixedCompilationDatabase Compilations("/", {"-MD", "-c", "-MMD", "-w"});
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -328,10 +328,11 @@
 
 ClangTool::ClangTool(const CompilationDatabase &Compilations,
  ArrayRef SourcePaths,
- std::shared_ptr PCHContainerOps)
+ std::shared_ptr PCHContainerOps,
+ IntrusiveRefCntPtr BaseFS)
 : Compilations(Compilations), SourcePaths(SourcePaths),
   PCHContainerOps(std::move(PCHContainerOps)),
-  OverlayFileSystem(new vfs::OverlayFileSystem(vfs::getRealFileSystem())),
+  OverlayFileSystem(new vfs::OverlayFileSystem(BaseFS)),
   InMemoryFileSystem(new vfs::InMemoryFileSystem),
   Files(new FileManager(FileSystemOptions(), Overl

[PATCH] D41947: Provide default virtual filesystem argument to ClangTool constructor

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

In https://reviews.llvm.org/D41947#980298, @ilya-biryukov wrote:

> Looks good. Do you have commit access or do you need someone to land this 
> patch for you?


No, i don't have commit access.


Repository:
  rC Clang

https://reviews.llvm.org/D41947



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

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 130385.
vladimir.plyashkun added a comment.

Moved logic to `ClangTidyMain`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/tool/ClangTidyMain.cpp

Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -17,6 +17,7 @@
 
 #include "../ClangTidy.h"
 #include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Frontend/FrontendDiagnostic.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/TargetSelect.h"
 
@@ -209,6 +210,12 @@
cl::init(false),
cl::cat(ClangTidyCategory));
 
+static cl::opt VfsOverlay("vfsoverlay", cl::desc(R"(
+Overlay the virtual filesystem described by file over the real file system.
+)"),
+   cl::value_desc("filename"),
+   cl::cat(ClangTidyCategory));
+
 namespace clang {
 namespace tidy {
 
@@ -330,6 +337,24 @@
 OverrideOptions);
 }
 
+void pushVfsOverlayFromFile(const std::string &OverlayFile,
+vfs::OverlayFileSystem &OverlayFS) {
+  llvm::ErrorOr> Buffer =
+  OverlayFS.getBufferForFile(OverlayFile);
+  if (!Buffer) {
+llvm::errs() << diag::err_missing_vfs_overlay_file << OverlayFile;
+return;
+  }
+
+  IntrusiveRefCntPtr FS = vfs::getVFSFromYAML(
+  std::move(Buffer.get()), /*DiagHandler*/ nullptr, OverlayFile);
+  if (!FS.get()) {
+llvm::errs() << diag::err_invalid_vfs_overlay << OverlayFile;
+return;
+  }
+  OverlayFS.pushOverlay(FS);
+}
+
 static int clangTidyMain(int argc, const char **argv) {
   CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
 cl::ZeroOrMore);
@@ -402,15 +427,21 @@
 return 0;
   }
 
+  llvm::IntrusiveRefCntPtr BaseFS(
+  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+  if (!VfsOverlay.empty()) {
+pushVfsOverlayFromFile(VfsOverlay, *BaseFS);
+  }
+
   ProfileData Profile;
 
   llvm::InitializeAllTargetInfos();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllAsmParsers();
 
   ClangTidyContext Context(std::move(OwningOptionsProvider));
   runClangTidy(Context, OptionsParser.getCompilations(), PathList,
-   EnableCheckProfile ? &Profile : nullptr);
+   BaseFS, EnableCheckProfile ? &Profile : nullptr);
   ArrayRef Errors = Context.getErrors();
   bool FoundErrors =
   std::find_if(Errors.begin(), Errors.end(), [](const ClangTidyError &E) {
@@ -422,7 +453,7 @@
   unsigned WErrorCount = 0;
 
   // -fix-errors implies -fix.
-  handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount);
+  handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount, BaseFS);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
 std::error_code EC;
Index: clang-tidy/ClangTidy.h
===
--- clang-tidy/ClangTidy.h
+++ clang-tidy/ClangTidy.h
@@ -227,6 +227,7 @@
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
   const tooling::CompilationDatabase &Compilations,
   ArrayRef InputFiles,
+  llvm::IntrusiveRefCntPtr BaseFS,
   ProfileData *Profile = nullptr);
 
 // FIXME: This interface will need to be significantly extended to be useful.
@@ -236,7 +237,8 @@
 /// Errors containing fixes are automatically applied and reformatted. If no
 /// clang-format configuration file is found, the given \P FormatStyle is used.
 void handleErrors(ClangTidyContext &Context, bool Fix,
-  unsigned &WarningsAsErrorsCount);
+  unsigned &WarningsAsErrorsCount,
+  llvm::IntrusiveRefCntPtr BaseFS);
 
 /// \brief Serializes replacements into YAML and writes them to the specified
 /// output stream.
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -89,8 +89,10 @@
 
 class ErrorReporter {
 public:
-  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes)
-  : Files(FileSystemOptions()), DiagOpts(new DiagnosticOptions()),
+  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
+llvm::IntrusiveRefCntPtr BaseFS)
+  : Files(FileSystemOptions(), BaseFS), 
+DiagOpts(new DiagnosticOptions()),
 DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
 Diags(IntrusiveRefCntPtr(new DiagnosticIDs), &*DiagOpts,
   DiagPrinter),
@@ -474,8 +476,11 @@
 
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
   const CompilationDatabase &Compilations,
-  ArrayRef InputFiles, ProfileData *Profile) {
-  ClangTool Tool

[PATCH] D41947: Provide default virtual filesystem argument to ClangTool constructor

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 130382.
vladimir.plyashkun added a comment.

Implemented test-case to check that `BaseFS` is actually used in `ClangTool`


Repository:
  rC Clang

https://reviews.llvm.org/D41947

Files:
  include/clang/Tooling/Tooling.h
  lib/Tooling/Tooling.cpp
  unittests/Tooling/ToolingTest.cpp


Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -402,6 +402,24 @@
   EXPECT_FALSE(Found);
 }
 
+TEST(ClangToolTest, BaseVirtualFileSystemUsage) {
+  FixedCompilationDatabase Compilations("/", std::vector());
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+
+  InMemoryFileSystem->addFile(
+  "a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+
+  ClangTool Tool(Compilations, std::vector(1, "a.cpp"),
+ std::make_shared(), 
OverlayFileSystem);
+  std::unique_ptr Action(
+  newFrontendActionFactory());
+  EXPECT_EQ(0, Tool.run(Action.get()));
+}
+
 // Check getClangStripDependencyFileAdjuster doesn't strip args after -MD/-MMD.
 TEST(ClangToolTest, StripDependencyFileAdjuster) {
   FixedCompilationDatabase Compilations("/", {"-MD", "-c", "-MMD", "-w"});
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -328,10 +328,11 @@
 
 ClangTool::ClangTool(const CompilationDatabase &Compilations,
  ArrayRef SourcePaths,
- std::shared_ptr PCHContainerOps)
+ std::shared_ptr PCHContainerOps,
+ IntrusiveRefCntPtr BaseFS)
 : Compilations(Compilations), SourcePaths(SourcePaths),
   PCHContainerOps(std::move(PCHContainerOps)),
-  OverlayFileSystem(new vfs::OverlayFileSystem(vfs::getRealFileSystem())),
+  OverlayFileSystem(new vfs::OverlayFileSystem(BaseFS)),
   InMemoryFileSystem(new vfs::InMemoryFileSystem),
   Files(new FileManager(FileSystemOptions(), OverlayFileSystem)),
   DiagConsumer(nullptr) {
Index: include/clang/Tooling/Tooling.h
===
--- include/clang/Tooling/Tooling.h
+++ include/clang/Tooling/Tooling.h
@@ -296,10 +296,13 @@
   ///not found in Compilations, it is skipped.
   /// \param PCHContainerOps The PCHContainerOperations for loading and 
creating
   /// clang modules.
+  /// \param BaseFS Base virtual filesystem used for OverlayFileSystem creation
   ClangTool(const CompilationDatabase &Compilations,
 ArrayRef SourcePaths,
 std::shared_ptr PCHContainerOps =
-std::make_shared());
+std::make_shared(),
+IntrusiveRefCntPtr BaseFS = 
+vfs::getRealFileSystem());
 
   ~ClangTool();
 


Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -402,6 +402,24 @@
   EXPECT_FALSE(Found);
 }
 
+TEST(ClangToolTest, BaseVirtualFileSystemUsage) {
+  FixedCompilationDatabase Compilations("/", std::vector());
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+
+  InMemoryFileSystem->addFile(
+  "a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+
+  ClangTool Tool(Compilations, std::vector(1, "a.cpp"),
+ std::make_shared(), OverlayFileSystem);
+  std::unique_ptr Action(
+  newFrontendActionFactory());
+  EXPECT_EQ(0, Tool.run(Action.get()));
+}
+
 // Check getClangStripDependencyFileAdjuster doesn't strip args after -MD/-MMD.
 TEST(ClangToolTest, StripDependencyFileAdjuster) {
   FixedCompilationDatabase Compilations("/", {"-MD", "-c", "-MMD", "-w"});
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -328,10 +328,11 @@
 
 ClangTool::ClangTool(const CompilationDatabase &Compilations,
  ArrayRef SourcePaths,
- std::shared_ptr PCHContainerOps)
+ std::shared_ptr PCHContainerOps,
+ IntrusiveRefCntPtr BaseFS)
 : Compilations(Compilations), SourcePaths(SourcePaths),
   PCHContainerOps(std::move(PCHContainerOps)),
-  OverlayFileSystem(new vfs::OverlayFileSystem(vfs::getRealFileSystem())),
+  OverlayFileSystem(new vfs::OverlayFileSystem(BaseFS)),
   InMemoryFileSystem(new vfs::InMemoryFileSystem),
   Files

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

Friendly ping


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535



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

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

Second part here (provided default virtual filesystem argument to ClangTool 
constructor): https://reviews.llvm.org/D41947


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535



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

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 129440.
vladimir.plyashkun added a reviewer: ilya-biryukov.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/ClangTidyOptions.cpp
  clang-tidy/ClangTidyOptions.h
  clang-tidy/tool/ClangTidyMain.cpp

Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -209,6 +209,12 @@
cl::init(false),
cl::cat(ClangTidyCategory));
 
+static cl::opt VfsOverlay("vfsoverlay", cl::desc(R"(
+Overlay the virtual filesystem described by file over the real file system.
+)"),
+   cl::value_desc("filename"),
+   cl::cat(ClangTidyCategory));
+
 namespace clang {
 namespace tidy {
 
@@ -295,6 +301,7 @@
   DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
   DefaultOptions.FormatStyle = FormatStyle;
   DefaultOptions.User = llvm::sys::Process::GetEnv("USER");
+  DefaultOptions.VfsOverlay = VfsOverlay;
   // USERNAME is used on Windows.
   if (!DefaultOptions.User)
 DefaultOptions.User = llvm::sys::Process::GetEnv("USERNAME");
@@ -312,6 +319,8 @@
 OverrideOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
   if (FormatStyle.getNumOccurrences() > 0)
 OverrideOptions.FormatStyle = FormatStyle;
+  if (VfsOverlay.getNumOccurrences() > 0)
+OverrideOptions.VfsOverlay = VfsOverlay;
 
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
Index: clang-tidy/ClangTidyOptions.h
===
--- clang-tidy/ClangTidyOptions.h
+++ clang-tidy/ClangTidyOptions.h
@@ -108,6 +108,10 @@
 
   /// \brief Add extra compilation arguments to the start of the list.
   llvm::Optional ExtraArgsBefore;
+
+  /// \brief Overlay the virtual filesystem described by file 
+  ///over the real file system
+  llvm::Optional VfsOverlay;
 };
 
 /// \brief Abstract interface for retrieving various ClangTidy options.
Index: clang-tidy/ClangTidyOptions.cpp
===
--- clang-tidy/ClangTidyOptions.cpp
+++ clang-tidy/ClangTidyOptions.cpp
@@ -92,6 +92,7 @@
 IO.mapOptional("CheckOptions", NOpts->Options);
 IO.mapOptional("ExtraArgs", Options.ExtraArgs);
 IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
+IO.mapOptional("VfsOverlay", Options.VfsOverlay);
   }
 };
 
@@ -152,6 +153,7 @@
   overrideValue(Result.User, Other.User);
   mergeVectors(Result.ExtraArgs, Other.ExtraArgs);
   mergeVectors(Result.ExtraArgsBefore, Other.ExtraArgsBefore);
+  overrideValue(Result.VfsOverlay, Other.VfsOverlay);
 
   for (const auto &KeyValue : Other.CheckOptions)
 Result.CheckOptions[KeyValue.first] = KeyValue.second;
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -166,6 +166,9 @@
   /// \brief Returns all collected errors.
   ArrayRef getErrors() const { return Errors; }
 
+  /// \brief Returns diagnostics engine
+  DiagnosticsEngine &getDiagnosticsEngine() const { return *DiagEngine; }
+
   /// \brief Clears collected errors.
   void clearErrors() { Errors.clear(); }
 
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -87,17 +87,46 @@
   ClangTidyContext &Context;
 };
 
+void pushVfsOverlayFromFile(StringRef OverlayFile,
+DiagnosticsEngine &Diags,
+vfs::OverlayFileSystem &OverlayFS) {
+  if (OverlayFile.empty())
+return;
+
+  llvm::ErrorOr> Buffer =
+  OverlayFS.getBufferForFile(OverlayFile);
+  if (!Buffer) {
+Diags.Report(diag::err_missing_vfs_overlay_file) << OverlayFile;
+return;
+  }
+
+  IntrusiveRefCntPtr FS = vfs::getVFSFromYAML(
+  std::move(Buffer.get()), /*DiagHandler*/ nullptr, OverlayFile);
+  if (!FS.get()) {
+Diags.Report(diag::err_invalid_vfs_overlay) << OverlayFile;
+return;
+  }
+  OverlayFS.pushOverlay(FS);
+}
+
 class ErrorReporter {
 public:
   ErrorReporter(ClangTidyContext &Context, bool ApplyFixes)
-  : Files(FileSystemOptions()), DiagOpts(new DiagnosticOptions()),
+  : OverlayFS(new vfs::OverlayFileSystem(vfs::getRealFileSystem())),
+Files(FileSystemOptions(), OverlayFS), 
+DiagOpts(new DiagnosticOptions()),
 DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
 Diags(IntrusiveRefCntPtr(new DiagnosticIDs), &*DiagOpts,
   DiagPrinter),
 SourceMgr(Diags, Files), Context(Context), ApplyFixes(ApplyFixes),
 TotalFixes(0), AppliedFixes(0), WarningsAsErrors(0) {
 

[PATCH] D41947: Provide default virtual filesystem argument in ClangTool constructor

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun created this revision.
vladimir.plyashkun added reviewers: alexfh, ilya-biryukov.
vladimir.plyashkun added a project: clang.
Herald added a subscriber: klimek.

This revision is part of another review.
Check this link for more details - https://reviews.llvm.org/D41535


Repository:
  rC Clang

https://reviews.llvm.org/D41947

Files:
  include/clang/Tooling/Tooling.h
  lib/Tooling/Tooling.cpp


Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -328,10 +328,11 @@
 
 ClangTool::ClangTool(const CompilationDatabase &Compilations,
  ArrayRef SourcePaths,
- std::shared_ptr PCHContainerOps)
+ std::shared_ptr PCHContainerOps,
+ IntrusiveRefCntPtr BaseFS)
 : Compilations(Compilations), SourcePaths(SourcePaths),
   PCHContainerOps(std::move(PCHContainerOps)),
-  OverlayFileSystem(new vfs::OverlayFileSystem(vfs::getRealFileSystem())),
+  OverlayFileSystem(new vfs::OverlayFileSystem(BaseFS)),
   InMemoryFileSystem(new vfs::InMemoryFileSystem),
   Files(new FileManager(FileSystemOptions(), OverlayFileSystem)),
   DiagConsumer(nullptr) {
Index: include/clang/Tooling/Tooling.h
===
--- include/clang/Tooling/Tooling.h
+++ include/clang/Tooling/Tooling.h
@@ -296,10 +296,13 @@
   ///not found in Compilations, it is skipped.
   /// \param PCHContainerOps The PCHContainerOperations for loading and 
creating
   /// clang modules.
+  /// \param BaseFS Base virtual filesystem used for OverlayFileSystem creation
   ClangTool(const CompilationDatabase &Compilations,
 ArrayRef SourcePaths,
 std::shared_ptr PCHContainerOps =
-std::make_shared());
+std::make_shared(),
+IntrusiveRefCntPtr BaseFS = 
+vfs::getRealFileSystem());
 
   ~ClangTool();
 


Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -328,10 +328,11 @@
 
 ClangTool::ClangTool(const CompilationDatabase &Compilations,
  ArrayRef SourcePaths,
- std::shared_ptr PCHContainerOps)
+ std::shared_ptr PCHContainerOps,
+ IntrusiveRefCntPtr BaseFS)
 : Compilations(Compilations), SourcePaths(SourcePaths),
   PCHContainerOps(std::move(PCHContainerOps)),
-  OverlayFileSystem(new vfs::OverlayFileSystem(vfs::getRealFileSystem())),
+  OverlayFileSystem(new vfs::OverlayFileSystem(BaseFS)),
   InMemoryFileSystem(new vfs::InMemoryFileSystem),
   Files(new FileManager(FileSystemOptions(), OverlayFileSystem)),
   DiagConsumer(nullptr) {
Index: include/clang/Tooling/Tooling.h
===
--- include/clang/Tooling/Tooling.h
+++ include/clang/Tooling/Tooling.h
@@ -296,10 +296,13 @@
   ///not found in Compilations, it is skipped.
   /// \param PCHContainerOps The PCHContainerOperations for loading and creating
   /// clang modules.
+  /// \param BaseFS Base virtual filesystem used for OverlayFileSystem creation
   ClangTool(const CompilationDatabase &Compilations,
 ArrayRef SourcePaths,
 std::shared_ptr PCHContainerOps =
-std::make_shared());
+std::make_shared(),
+IntrusiveRefCntPtr BaseFS = 
+vfs::getRealFileSystem());
 
   ~ClangTool();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41594: Support `ivfsoverlay` option in Tooling

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added inline comments.



Comment at: lib/Tooling/Tooling.cpp:287
+  if (Files->getVirtualFileSystem() != VirtualFileSystem) {
+Files = new FileManager(Invocation->getFileSystemOpts(), 
VirtualFileSystem);
+  }

ilya-biryukov wrote:
> `Files` is a raw pointer, so we're leaking memory here.
> 
Agree.
I can try to replace type of this field to `llvm::IntrusiveRefCntPtr`  
instead of raw pointer.
But if i understand correctly, this class is available during whole execution, 
so memory will be freed on exit. 
I saw some other usages, for example, `createFileManager` method in the 
`CompilerInstance` also just reassign value to raw pointer.
https://clang.llvm.org/doxygen/classclang_1_1CompilerInstance.html#abeb2bbf46a8de987c227125a84935802


Repository:
  rC Clang

https://reviews.llvm.org/D41594



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

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

FYI, i've create revision to support `ivfsoverlay` option in Tooling: 
https://reviews.llvm.org/D41594


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535



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

[PATCH] D41594: Support `ivfsoverlay` option in Tooling

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun created this revision.
vladimir.plyashkun added reviewers: bkramer, alexfh, ilya-biryukov.
vladimir.plyashkun added a project: clang.
Herald added a subscriber: klimek.

Previously, this argument had no effect, since it didn't proceeded.
For more information, check this review: https://reviews.llvm.org/D41535


Repository:
  rC Clang

https://reviews.llvm.org/D41594

Files:
  lib/Frontend/CompilerInvocation.cpp
  lib/Tooling/Tooling.cpp
  unittests/Tooling/ToolingTest.cpp


Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -280,6 +280,12 @@
 Invocation->getPreprocessorOpts().addRemappedFile(It.getKey(),
   Input.release());
   }
+  IntrusiveRefCntPtr VirtualFileSystem =
+  createVFSFromCompilerInvocation(*Invocation, Diagnostics,
+  Files->getVirtualFileSystem());
+  if (Files->getVirtualFileSystem() != VirtualFileSystem) {
+Files = new FileManager(Invocation->getFileSystemOpts(), 
VirtualFileSystem);
+  }
   return runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
std::move(PCHContainerOps));
 }
Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -21,6 +21,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "gtest/gtest.h"
 #include 
 #include 
@@ -171,6 +172,37 @@
   EXPECT_TRUE(Invocation.run());
 }
 
+TEST(ToolInvocation, TestVfsOverlay) {
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr Files(
+  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  std::vector Args;
+  Args.push_back("tool-executable");
+  Args.push_back("-ivfsoverlay");
+  Args.push_back("overlay.yaml");
+  Args.push_back("-fsyntax-only");
+  Args.push_back("a.cpp");
+  clang::tooling::ToolInvocation Invocation(Args, new SyntaxOnlyAction,
+Files.get());
+  InMemoryFileSystem->addFile(
+  "a.cpp", 0,
+  llvm::MemoryBuffer::getMemBuffer("#include \n"));
+  std::string OverlayContent = llvm::formatv(
+  "{{'version': 0,'roots':[{{'name':'{0}','type':'directory',"
+ "'contents':[{{'name':'a.cpp','type':'file',"
+ "'external-contents':'b.cpp'}]}]}",
+  InMemoryFileSystem.get()->getCurrentWorkingDirectory().get());
+  InMemoryFileSystem->addFile("overlay.yaml", 0,
+  
llvm::MemoryBuffer::getMemBuffer(OverlayContent));
+  InMemoryFileSystem->addFile(
+  "b.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+  EXPECT_TRUE(Invocation.run());
+}
+
 TEST(ToolInvocation, TestVirtualModulesCompilation) {
   // FIXME: Currently, this only tests that we don't exit with an error if a
   // mapped module.map is found on the include path. In the future, expand this
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2988,8 +2988,9 @@
   return IntrusiveRefCntPtr();
 }
 
-IntrusiveRefCntPtr FS = vfs::getVFSFromYAML(
-std::move(Buffer.get()), /*DiagHandler*/ nullptr, File);
+IntrusiveRefCntPtr FS =
+vfs::getVFSFromYAML(std::move(Buffer.get()), /*DiagHandler*/ nullptr,
+File, /*DiagContext*/ nullptr, BaseFS);
 if (!FS.get()) {
   Diags.Report(diag::err_invalid_vfs_overlay) << File;
   return IntrusiveRefCntPtr();


Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -280,6 +280,12 @@
 Invocation->getPreprocessorOpts().addRemappedFile(It.getKey(),
   Input.release());
   }
+  IntrusiveRefCntPtr VirtualFileSystem =
+  createVFSFromCompilerInvocation(*Invocation, Diagnostics,
+  Files->getVirtualFileSystem());
+  if (Files->getVirtualFileSystem() != VirtualFileSystem) {
+Files = new FileManager(Invocation->getFileSystemOpts(), VirtualFileSystem);
+  }
   return runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
std::move(PCHContainerOps));
 }
Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -21,6 +21,7 

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

@ilya-biryukov 
Yes, this is exactly what i need.
Unfortunately, `-ivfsoverlay` in the compile commands works for the compiler 
invocation, but it doesn't work for tooling.
E.g. this call:

  clang-tidy -checks=*  -- -ivfsoverlay=

has no effect.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535



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

[PATCH] D41536: Provide public getter for OverlayFileSystem

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun created this revision.
vladimir.plyashkun added a reviewer: alexfh.
vladimir.plyashkun added projects: clang, clang-tools-extra.
Herald added subscribers: cfe-commits, klimek.

This is part of review - https://reviews.llvm.org/D41535


Repository:
  rC Clang

https://reviews.llvm.org/D41536

Files:
  include/clang/Tooling/Tooling.h


Index: include/clang/Tooling/Tooling.h
===
--- include/clang/Tooling/Tooling.h
+++ include/clang/Tooling/Tooling.h
@@ -339,6 +339,10 @@
 
   llvm::ArrayRef getSourcePaths() const { return SourcePaths; }
 
+  vfs::OverlayFileSystem &getOverlayFileSystem() const {
+return *OverlayFileSystem;
+  }
+
 private:
   const CompilationDatabase &Compilations;
   std::vector SourcePaths;


Index: include/clang/Tooling/Tooling.h
===
--- include/clang/Tooling/Tooling.h
+++ include/clang/Tooling/Tooling.h
@@ -339,6 +339,10 @@
 
   llvm::ArrayRef getSourcePaths() const { return SourcePaths; }
 
+  vfs::OverlayFileSystem &getOverlayFileSystem() const {
+return *OverlayFileSystem;
+  }
+
 private:
   const CompilationDatabase &Compilations;
   std::vector SourcePaths;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

Second part of review - https://reviews.llvm.org/D41536


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535



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

[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun created this revision.
vladimir.plyashkun added reviewers: alexfh, benlangmuir.
vladimir.plyashkun added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

These changes introduce support for -vfsoverlay option in Clang-Tidy
The main reason for it:
In IDE integration we have two different pieces of source: editor and physical 
file.
File is placed in some project directory (let's call it "A")
We want to run clang-tidy exactly in this project directory "A",  in order to 
get all .clang-tidy files in parent directories along with correct resolution 
for all relative include paths.
But in IDE, saving mechanism perfoms periodically.
So, to perform analysis of actual text on the fly, we need to save document on 
every modification  (which is inefficient) or generate file in the temporary 
directory with actual text (it also leads to skipping .clang-tidy files and 
complicates include search paths resolution)
-vfsoverlay approach allows us to analyze source file in the project directory 
"A" but with actual data placed in another directory.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyOptions.cpp
  clang-tidy/ClangTidyOptions.h
  clang-tidy/tool/ClangTidyMain.cpp

Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -209,6 +209,12 @@
cl::init(false),
cl::cat(ClangTidyCategory));
 
+static cl::opt VfsOverlay("vfsoverlay", cl::desc(R"(
+Overlay the virtual filesystem described by file over the real file system.
+)"),
+   cl::value_desc("filename"),
+   cl::cat(ClangTidyCategory));
+
 namespace clang {
 namespace tidy {
 
@@ -295,6 +301,7 @@
   DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
   DefaultOptions.FormatStyle = FormatStyle;
   DefaultOptions.User = llvm::sys::Process::GetEnv("USER");
+  DefaultOptions.VfsOverlay = VfsOverlay;
   // USERNAME is used on Windows.
   if (!DefaultOptions.User)
 DefaultOptions.User = llvm::sys::Process::GetEnv("USERNAME");
@@ -312,6 +319,8 @@
 OverrideOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
   if (FormatStyle.getNumOccurrences() > 0)
 OverrideOptions.FormatStyle = FormatStyle;
+  if (VfsOverlay.getNumOccurrences() > 0)
+OverrideOptions.VfsOverlay = VfsOverlay;
 
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
@@ -331,7 +340,16 @@
 }
 
 static int clangTidyMain(int argc, const char **argv) {
-  CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
+  llvm::SpecificBumpPtrAllocator ArgAllocator;
+  SmallVector Argv;
+  std::error_code EC = llvm::sys::Process::GetArgumentVector(
+  Argv, llvm::makeArrayRef(argv, argc), ArgAllocator);
+  if (EC) {
+llvm::errs() << "error: couldn't get arguments: " << EC.message() << '\n';
+return 1;
+  }
+
+  CommonOptionsParser OptionsParser(argc, &Argv[0], ClangTidyCategory,
 cl::ZeroOrMore);
 
   auto OwningOptionsProvider = createOptionsProvider();
Index: clang-tidy/ClangTidyOptions.h
===
--- clang-tidy/ClangTidyOptions.h
+++ clang-tidy/ClangTidyOptions.h
@@ -108,6 +108,8 @@
 
   /// \brief Add extra compilation arguments to the start of the list.
   llvm::Optional ExtraArgsBefore;
+
+  llvm::Optional VfsOverlay;
 };
 
 /// \brief Abstract interface for retrieving various ClangTidy options.
Index: clang-tidy/ClangTidyOptions.cpp
===
--- clang-tidy/ClangTidyOptions.cpp
+++ clang-tidy/ClangTidyOptions.cpp
@@ -92,6 +92,7 @@
 IO.mapOptional("CheckOptions", NOpts->Options);
 IO.mapOptional("ExtraArgs", Options.ExtraArgs);
 IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
+IO.mapOptional("VfsOverlay", Options.VfsOverlay);
   }
 };
 
@@ -152,6 +153,7 @@
   overrideValue(Result.User, Other.User);
   mergeVectors(Result.ExtraArgs, Other.ExtraArgs);
   mergeVectors(Result.ExtraArgsBefore, Other.ExtraArgsBefore);
+  overrideValue(Result.VfsOverlay, Other.VfsOverlay);
 
   for (const auto &KeyValue : Other.CheckOptions)
 Result.CheckOptions[KeyValue.first] = KeyValue.second;
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -87,17 +87,41 @@
   ClangTidyContext &Context;
 };
 
+void pushVfsOverlayFromFile(StringRef OverlayFile,
+vfs::OverlayFileSystem &OverlayFS) {
+  if (OverlayFile.empty())
+return;
+
+  llvm::ErrorOr> Buffer =
+  OverlayFS.getBufferForFile(OverlayFile);
+  if (!Buffer) {
+return;
+  }
+
+  IntrusiveRefCntPtr FS = vfs::getVFSFromYAML(
+  std::move(Buffer.g

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

@alexf
Thanks for the response!
Yes, we re-implemented logic and now use JSON compilation database to pass 
compiler options to Clang-Tidy.
Anyway, i think in general it's useful to support @response_files, see my 
comment: https://reviews.llvm.org/D34440#813411


Repository:
  rL LLVM

https://reviews.llvm.org/D34440



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

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

> Many build systems normally generate response files on-fly in some 
> circumstances (e.g. if command line is longer than some platform-imposed 
> limit). So IMO response files should be a perfect citizen here.

friendly ping


Repository:
  rL LLVM

https://reviews.llvm.org/D34440



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

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

Thanks for the response @klimek !
Sorry for possible confusions.

> Yes. I'm still confused why in this case clang-tidy @file --  would be 
> expected to expand the response file? Am I missing something?

I think that we discussed use-case when @response files can appear in the 
compiler options (right after `--` symbol).

> I don't understand this. Can you elaborate?

As IDE developers we can't control how users pass options to compiler.
For example, user can set compiler options directly in the CMake, like this:

  set(CMAKE_CXX_FLAGS "@response.rsp")

and our goal is to pass this options to Clang-Tidy as is. 
It doesn't matter how will we pass them to Clang-Tidy through // 
compile_command.json//:

  {
"directory" : "",
 "command": "clang++ ... @response.rsp",
 "file": "..."
  }

or through command line (we choosed this method):

  clang-tidy ... -- @response.rsp

If compiler options will contain response files, all contents will be 
discarded, because Clang based tools don't expand response files.
By this fact we can loose important compiler information (header paths, 
definitions,  etc.) which will lead us to inaccurate analysis result.


Repository:
  rL LLVM

https://reviews.llvm.org/D34440



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

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

> If you want one unified format, the compilation database is it.

Is the `clang-tidy ... -- ` meant to be more or less a drop-in 
replacement for `clang ` (arguments-wise)? 
If yes, this expansion of response files here is an another step in this 
direction.

Our only concerns are the internal ones (instead of using our well-tested 
response file implementation, we have to do something similar but different 
with JSON-based compilation database format). 
Obviously, it is possible to use it, and if this patch won't be accepted, we 
would. 
But then, again, I see this change as a positive by itself and don't really 
understand why the compiler driver should expand response arguments and the 
fixed compilation database shouldn't.

As IDE developers we don't have full control on the format of compiler options. 
They come from users in free form which compiler can understand.
From the implementation view it would be more transparent and efficient to 
transfer them in the original form to Clang-Tidy (instead of generating 
intermediate files).


Repository:
  rL LLVM

https://reviews.llvm.org/D34440



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

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

> Are there any concerns using the alternative?

I can't say that it's a big problems, but i think that:
//CompilationDatabase.json// is more //CMake //specific format. 
It can be generated automatically by //CMake//, while other build systems may 
not do it.
So we need to generate it on the fly (by tool or by hand), which also can lead 
to hidden problems due to different formats, different escaping rules, etc.
I think it's always good to have one unified format.


Repository:
  rL LLVM

https://reviews.llvm.org/D34440



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

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

Even if i'll change content of //arguments.rsp// to
`-std=c++11 -Ipath/to/include -Ipath/to/include2 -DMACRO `
and will try to call clang-tidy process in this way:
`clang-tidy -checks=* main.cpp -export-fixes=... -- @arguments.rsp`
it also has no effect, because all compiler options will be ignored (i thinks 
it's because that //stripPositionalArgs()// function deletes @arguments.rsp 
parameter as unused input).


Repository:
  rL LLVM

https://reviews.llvm.org/D34440



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

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

Thanks @klimek for the response!
Let me give an example.
Suppose we want to check //main.cpp// by clang-tidy.
As i said before, i cannot use //CompilationDatabase.json// (due to some 
technical reasons), 
but there is way to pass all options though command line and call external 
process of clang-tidy in this way:
`clang-tidy -checks=* main.cpp -export-fixes=... --  -std=c++11 
-Ipath/to/include -Ipath/to/include2 -DMACRO `
All compiler related options appear in the command line after `--` symbol.
But due to command line length restrictions (especially on Windows OS) it's 
better to use //@ResponseFile// mechanism.
To achieve it, i've created //arguments.rsp// file with this content:
`-checks=* main.cpp -export-fixes=... --  -std=c++11 -Ipath/to/include 
-Ipath/to/include2 -DMACRO `
and tried to call clang-tidy in this way:
`clang-tidy @arguments.rsp`
But all compiler options are ignored by this call!
If you check how //CommonOptionsParser.cpp// parses command line arguments, you 
will see, that first of all, it tries to load fixed compilation database (by 
finding `--` symbol in the command line).
Only after that it calls //cl::ParseCommandLineOptions// which loads content of 
//arguments.rsp// file and parses all the rest arguments.
So, my question was why not to expand response files before loading fixed 
compilation db from command line?
Or i'm doing something wrong?


Repository:
  rL LLVM

https://reviews.llvm.org/D34440



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

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

**To discuss:**
This patch is required for correct integration //Clang-Tidy// with //CLion 
IDE//.
By this moment, we unable to use //CompilationDatabase.json// from //CLion 
//side which is widely used in //Clang-Tidy// and in other common tools.
Anyway, there are possibility to pass compiler options directly through command 
line.
But we cannot pass compiler options directly through command-line, due to 
command-line-length restrictions.
So, we decided to pass compiler options through //@ResponseFile //mechanism.
But there are some problems with this approach.
Before this patch, ///clang/lib/Tooling/CommonOptionsParser.cpp// worked in 
this way:

1. Load compilation database from command-line
2. Expand response files
3. Parse all other command line arguments

I think it's strange behavior? 
Why we try to load compilation database first? 
Maybe it's better to expand response files and only after that try to load 
compilation database and parse another options?
It's hard to refactor //cl::ParseCommandLineOptions// and 
//cl::ExpandResponseFiles// functions, due to high number of usages, so i 
copied some block directly into ///clang/lib/Tooling/CommonOptionsParser.cpp//
I don't think that is a best solution, but i don't have another one.




Comment at: unittests/Tooling/CommonOptionsParserTest.cpp:41
+  ~CommonOptionsParserTest() override {
+llvm::sys::fs::remove(ResponseFileName);
+llvm::sys::fs::remove(TestDir);

I moved test-cases to separate file.
But as i mentioned before, i don't see any options how to use VFS here.
All other test-cases related with response files expansion also don't use VFS 
for some reasons, e.g. `unittests/Support/CommandLineTest.cpp`


Repository:
  rL LLVM

https://reviews.llvm.org/D34440



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

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 106436.
vladimir.plyashkun added a comment.

- trailing period
- moved test-cases to separate file


Repository:
  rL LLVM

https://reviews.llvm.org/D34440

Files:
  lib/Tooling/CommonOptionsParser.cpp
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/CommonOptionsParserTest.cpp

Index: unittests/Tooling/CommonOptionsParserTest.cpp
===
--- /dev/null
+++ unittests/Tooling/CommonOptionsParserTest.cpp
@@ -0,0 +1,72 @@
+//===- unittests/Tooling/CommonOptionsParserTest.cpp -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+
+#include 
+
+namespace clang {
+namespace tooling {
+
+class CommonOptionsParserTest : public testing::Test {
+public:
+  SmallString<128> TestDir;
+  SmallString<128> ResponseFileName;
+
+  CommonOptionsParserTest() {
+std::error_code EC = llvm::sys::fs::createUniqueDirectory("unittest", TestDir);
+EXPECT_TRUE(!EC);
+llvm::sys::path::append(ResponseFileName, TestDir, "resp");
+  }
+
+  void setResponseFileContent(const std::string &Content) {
+std::ofstream File(ResponseFileName.c_str());
+EXPECT_TRUE(File.is_open());
+File << Content;
+File.close();
+  }
+
+  ~CommonOptionsParserTest() override {
+llvm::sys::fs::remove(ResponseFileName);
+llvm::sys::fs::remove(TestDir);
+  }
+};
+
+TEST_F(CommonOptionsParserTest, ExpandResponseFilesBeforeLoadingCompilationDatabase) {
+  setResponseFileContent("-- -a=true -b");
+  SmallString<128> ResponseFileRef;
+  ResponseFileRef.append(1, '@');
+  ResponseFileRef.append(ResponseFileName.c_str());
+  int Argc = 3;
+  const char *Argv[] = { "1", "2", ResponseFileRef.c_str() };
+  llvm::cl::OptionCategory Category("options");
+  llvm::cl::opt option1("a", llvm::cl::desc(""), llvm::cl::init(false),
+  llvm::cl::cat(Category));
+  llvm::cl::opt option2("b", llvm::cl::desc(""), llvm::cl::init(false),
+  llvm::cl::cat(Category));
+  CommonOptionsParser Parser(Argc, Argv, Category);
+  CompilationDatabase &Database = Parser.getCompilations();
+  std::vector ActualCC = Database.getCompileCommands("source");
+  ASSERT_EQ(1u, ActualCC.size());
+  std::vector ExpectedCmd;
+  ExpectedCmd.push_back("clang-tool");
+  ExpectedCmd.push_back("-a=true");
+  ExpectedCmd.push_back("-b");
+  ExpectedCmd.push_back("source");
+
+  ASSERT_EQ(ExpectedCmd, ActualCC[0].CommandLine);
+}
+	
+} // end namespace tooling
+} // end namespace clang
Index: unittests/Tooling/CMakeLists.txt
===
--- unittests/Tooling/CMakeLists.txt
+++ unittests/Tooling/CMakeLists.txt
@@ -14,6 +14,7 @@
   CastExprTest.cpp
   CommentHandlerTest.cpp
   CompilationDatabaseTest.cpp
+  CommonOptionsParserTest.cpp
   DiagnosticsYamlTest.cpp
   FixItTest.cpp
   LookupTest.cpp
Index: lib/Tooling/CommonOptionsParser.cpp
===
--- lib/Tooling/CommonOptionsParser.cpp
+++ lib/Tooling/CommonOptionsParser.cpp
@@ -25,6 +25,7 @@
 //===--===//
 
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/StringSaver.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
@@ -116,6 +117,14 @@
 
   cl::HideUnrelatedOptions(Category);
 
+  // Expand response files before loading compilation database from command line.
+  SmallVector NewArgv(argv, argv + argc);
+  BumpPtrAllocator A;
+  StringSaver Saver(A);
+  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, NewArgv);
+  argv = &NewArgv[0];
+  argc = static_cast(NewArgv.size());
+
   std::string ErrorMessage;
   Compilations =
   FixedCompilationDatabase::loadFromCommandLine(argc, argv, ErrorMessage);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added inline comments.



Comment at: unittests/Tooling/CompilationDatabaseTest.cpp:638-652
+  ParseCompilationDatabaseFromResponseFileTest() {
+std::error_code EC = llvm::sys::fs::createUniqueDirectory("unittest", 
TestDir);
+EXPECT_TRUE(!EC);
+llvm::sys::path::append(ResponseFileName, TestDir, "resp");
+  }
+  void setResponseFileContent(const std::string &content) {
+std::ofstream File(ResponseFileName.c_str());

alexfh wrote:
> Can we do all this in the virtual file system 
> (include/clang/Basic/VirtualFileSystem.h) to avoid creating files from the 
> unit test?
Unfortunately, i don't see any options how to do it with VFS. 
All other tests related with response files expansion don't use VFS for some 
reasons, e.g. `unittests/Support/CommandLineTest.cpp`


Repository:
  rL LLVM

https://reviews.llvm.org/D34440



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

[PATCH] D35349: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

This is second part of review: 
https://reviews.llvm.org/D34404
which is contains changes related to clang-tools-extra repository.


Repository:
  rL LLVM

https://reviews.llvm.org/D35349



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

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

I've splitted single revision into two different revisions:
https://reviews.llvm.org/D34404
https://reviews.llvm.org/D35349
Also, i fixed failing test-cases in clang-apply-replacements due to changes in 
output format (so they shouldn't be a problem anymore)


Repository:
  rL LLVM

https://reviews.llvm.org/D34404



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

[PATCH] D35349: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun created this revision.
vladimir.plyashkun added a project: clang-tools-extra.
Herald added subscribers: xazax.hun, JDevlieghere, mgorny.

To get properly integration Clang-Tidy with CLion IDE, next things were 
implemented:

- Preserve `Message`, `FileOffset`, `FilePath` in the clang-tidy output.
- Export all diagnostics, not just the ones with fixes
- Test-cases


Repository:
  rL LLVM

https://reviews.llvm.org/D35349

Files:
  test/clang-apply-replacements/Inputs/basic/file1.yaml
  test/clang-apply-replacements/Inputs/basic/file2.yaml
  test/clang-apply-replacements/Inputs/conflict/file1.yaml
  test/clang-apply-replacements/Inputs/conflict/file2.yaml
  test/clang-apply-replacements/Inputs/conflict/file3.yaml
  test/clang-apply-replacements/Inputs/crlf/file1.yaml
  test/clang-apply-replacements/Inputs/format/no.yaml
  test/clang-apply-replacements/Inputs/format/yes.yaml
  unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  unittests/clang-apply-replacements/CMakeLists.txt

Index: unittests/clang-apply-replacements/CMakeLists.txt
===
--- unittests/clang-apply-replacements/CMakeLists.txt
+++ unittests/clang-apply-replacements/CMakeLists.txt
@@ -8,6 +8,7 @@
   )
 
 add_extra_unittest(ClangApplyReplacementsTests
+  ApplyReplacementsTest.cpp
   ReformattingTest.cpp
   )
 
Index: unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
===
--- /dev/null
+++ unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
@@ -0,0 +1,49 @@
+//===- clang-apply-replacements/ApplyReplacementsTest.cpp --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang-apply-replacements/Tooling/ApplyReplacements.h"
+#include "gtest/gtest.h"
+
+using namespace clang::replace;
+using namespace llvm;
+
+namespace clang { 
+namespace tooling {
+
+static TUDiagnostics makeTUDiagnostics(const std::string &MainSourceFile,
+   StringRef DiagnosticName,
+   const DiagnosticMessage &Message,
+   const StringMap &Replacements,
+   StringRef BuildDirectory) {
+  TUDiagnostics TUs;
+  TUs.push_back({ MainSourceFile, 
+  { { DiagnosticName, Message, Replacements, {}, 
+  Diagnostic::Warning, BuildDirectory } } 
+  });
+  return TUs;
+}
+
+// Test to ensure diagnostics with no fixes, will be merged correctly 
+// before applying.
+TEST(ApplyReplacementsTest, mergeDiagnosticsWithNoFixes) {
+  IntrusiveRefCntPtr DiagOpts(new DiagnosticOptions());
+  DiagnosticsEngine Diagnostics(
+IntrusiveRefCntPtr(new DiagnosticIDs()), DiagOpts.get());
+  FileManager Files((FileSystemOptions()));
+  SourceManager SM(Diagnostics, Files);
+  TUDiagnostics TUs = makeTUDiagnostics("path/to/source.cpp", "diagnostic",
+{}, {}, "path/to");
+  FileToReplacementsMap ReplacementsMap;
+
+  EXPECT_TRUE(mergeAndDeduplicate(TUs, ReplacementsMap, SM));
+  EXPECT_TRUE(ReplacementsMap.empty());
+}
+
+} // end namespace tooling
+} // end namespace clang
Index: test/clang-apply-replacements/Inputs/format/yes.yaml
===
--- test/clang-apply-replacements/Inputs/format/yes.yaml
+++ test/clang-apply-replacements/Inputs/format/yes.yaml
@@ -4,6 +4,9 @@
 MainSourceFile:  yes.cpp
 Diagnostics: 
   - DiagnosticName:  test-yes
+Message: Fix
+FilePath: $(path)/yes.cpp
+FileOffset: 494
 Replacements:
   - FilePath:$(path)/yes.cpp
 Offset:  494
Index: test/clang-apply-replacements/Inputs/format/no.yaml
===
--- test/clang-apply-replacements/Inputs/format/no.yaml
+++ test/clang-apply-replacements/Inputs/format/no.yaml
@@ -2,6 +2,9 @@
 MainSourceFile:  no.cpp
 Diagnostics: 
   - DiagnosticName:  test-no
+Message: Fix
+FilePath: $(path)/no.cpp
+FileOffset: 94
 Replacements:
   - FilePath:$(path)/no.cpp
 Offset:  94
Index: test/clang-apply-replacements/Inputs/crlf/file1.yaml
===
--- test/clang-apply-replacements/Inputs/crlf/file1.yaml
+++ test/clang-apply-replacements/Inputs/crlf/file1.yaml
@@ -2,6 +2,9 @@
 MainSourceFile:  source1.cpp
 Diagnostics:
   - DiagnosticName:  test-crlf
+Message: Fix
+FilePath: $(path)/crlf.cpp
+FileOffset: 79
 Replacements:
   - FilePath:$(path)/crlf.cpp
 Offset:  79
Index: test/clang-apply-replac

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 106402.
vladimir.plyashkun added a comment.

1. split revision into two
2. fix failing tests


Repository:
  rL LLVM

https://reviews.llvm.org/D34404

Files:
  include/clang/Tooling/DiagnosticsYaml.h
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/DiagnosticsYamlTest.cpp

Index: unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- /dev/null
+++ unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -0,0 +1,173 @@
+//===- unittests/Tooling/DiagnosticsYamlTest.cpp - Serialization tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Tests for serialization of Diagnostics.
+//
+//===--===//
+
+#include "clang/Tooling/Core/Diagnostic.h"
+#include "clang/Tooling/DiagnosticsYaml.h"
+#include "clang/Tooling/ReplacementsYaml.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang::tooling;
+
+static Diagnostic makeDiagnostic(StringRef DiagnosticName, 
+ const std::string &Message,
+ int FileOffset,
+ const std::string &FilePath,
+ const StringMap &Fix) {
+  DiagnosticMessage DiagMessage;
+  DiagMessage.Message = Message;
+  DiagMessage.FileOffset = FileOffset;
+  DiagMessage.FilePath = FilePath;
+  return Diagnostic(DiagnosticName, DiagMessage, Fix, {},
+Diagnostic::Warning, "path/to/build/directory");
+}
+
+TEST(DiagnosticsYamlTest, serializesDiagnostics) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+
+  StringMap Fix1 = { 
+{ 
+  "path/to/source.cpp",  
+  Replacements({ "path/to/source.cpp", 100, 12, "replacement #1" }) 
+} 
+  };
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55, 
+   "path/to/source.cpp", Fix1));
+  
+  StringMap Fix2 = {
+{ 
+  "path/to/header.h",
+  Replacements({ "path/to/header.h", 62, 2, "replacement #2" })
+} 
+  };
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60, 
+   "path/to/header.h", Fix2));
+
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72, 
+   "path/to/source2.cpp", {}));
+
+  std::string YamlContent;
+  raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << TUD;
+
+  EXPECT_EQ("---\n"
+"MainSourceFile:  path/to/source.cpp\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1\'\n"
+"Message: 'message #1'\n"
+"FileOffset:  55\n"
+"FilePath:path/to/source.cpp\n"
+"Replacements:\n"
+"  - FilePath:path/to/source.cpp\n"
+"Offset:  100\n"
+"Length:  12\n"
+"ReplacementText: 'replacement #1'\n"
+"  - DiagnosticName:  'diagnostic#2'\n"
+"Message: 'message #2'\n"
+"FileOffset:  60\n"
+"FilePath:path/to/header.h\n"
+"Replacements:\n"
+"  - FilePath:path/to/header.h\n"
+"Offset:  62\n"
+"Length:  2\n"
+"ReplacementText: 'replacement #2'\n"
+"  - DiagnosticName:  'diagnostic#3'\n"
+"Message: 'message #3'\n"
+"FileOffset:  72\n"
+"FilePath:path/to/source2.cpp\n"
+"Replacements:\n"
+"...\n",
+  YamlContentStream.str());
+}
+
+TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
+std::string YamlContent = "---\n"
+  "MainSourceFile:  path/to/source.cpp\n"
+  "Diagnostics: \n"
+  "  - DiagnosticName:  'diagnostic#1'\n"
+  "Message: 'message #1'\n"
+  "FileOffset:  55\n"
+  "FilePath:path/to/source.cpp\n"
+  "Replacements:\n"
+  "  - FilePath:path/to/source.cpp\n"
+  "Offset:  100\n"
+  "Length:  12\n"
+  "ReplacementText: 'replacement #1'\n"
+  "  - DiagnosticName:  'diagnostic#2'\n"
+  "Message: 'message #2'\n"
+  "FileOffset:  60\n"
+  "FilePath:path/to/header.h\n"
+  "Replacements:\n"
+  "  - FilePath:path/to/header.h\n"
+  "Offset:  62\n"
+  "Length:  2\n"
+  "ReplacementText: 'replacement #2'\n"
+  "  - DiagnosticName:  'diagnostic#3'\n"
+  "Message: 'message #3'\n"
+  "Fil

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 105283.
vladimir.plyashkun edited the summary of this revision.
vladimir.plyashkun added a comment.

- made code less verbose
- used assignment initialization form
- removed extra `.c_str()` call


Repository:
  rL LLVM

https://reviews.llvm.org/D34404

Files:
  include/clang/Tooling/DiagnosticsYaml.h
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/DiagnosticsYamlTest.cpp
  unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  unittests/clang-apply-replacements/CMakeLists.txt

Index: unittests/clang-apply-replacements/CMakeLists.txt
===
--- unittests/clang-apply-replacements/CMakeLists.txt
+++ unittests/clang-apply-replacements/CMakeLists.txt
@@ -8,6 +8,7 @@
   )
 
 add_extra_unittest(ClangApplyReplacementsTests
+  ApplyReplacementsTest.cpp
   ReformattingTest.cpp
   )
 
Index: unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
===
--- /dev/null
+++ unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
@@ -0,0 +1,49 @@
+//===- clang-apply-replacements/ApplyReplacementsTest.cpp --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang-apply-replacements/Tooling/ApplyReplacements.h"
+#include "gtest/gtest.h"
+
+using namespace clang::replace;
+using namespace llvm;
+
+namespace clang { 
+namespace tooling {
+
+static TUDiagnostics makeTUDiagnostics(const std::string &MainSourceFile,
+   StringRef DiagnosticName,
+   const DiagnosticMessage &Message,
+   const StringMap &Replacements,
+   StringRef BuildDirectory) {
+  TUDiagnostics TUs;
+  TUs.push_back({ MainSourceFile, 
+  { { DiagnosticName, Message, Replacements, {}, 
+  Diagnostic::Warning, BuildDirectory } } 
+  });
+  return TUs;
+}
+
+// Test to ensure diagnostics with no fixes, will be merged correctly 
+// before applying.
+TEST(ApplyReplacementsTest, mergeDiagnosticsWithNoFixes) {
+  IntrusiveRefCntPtr DiagOpts(new DiagnosticOptions());
+  DiagnosticsEngine Diagnostics(
+IntrusiveRefCntPtr(new DiagnosticIDs()), DiagOpts.get());
+  FileManager Files((FileSystemOptions()));
+  SourceManager SM(Diagnostics, Files);
+  TUDiagnostics TUs = makeTUDiagnostics("path/to/source.cpp", "diagnostic",
+{}, {}, "path/to");
+  FileToReplacementsMap ReplacementsMap;
+
+  EXPECT_TRUE(mergeAndDeduplicate(TUs, ReplacementsMap, SM));
+  EXPECT_TRUE(ReplacementsMap.empty());
+}
+
+} // end namespace tooling
+} // end namespace clang
Index: unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- /dev/null
+++ unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -0,0 +1,173 @@
+//===- unittests/Tooling/DiagnosticsYamlTest.cpp - Serialization tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Tests for serialization of Diagnostics.
+//
+//===--===//
+
+#include "clang/Tooling/Core/Diagnostic.h"
+#include "clang/Tooling/DiagnosticsYaml.h"
+#include "clang/Tooling/ReplacementsYaml.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang::tooling;
+
+static Diagnostic makeDiagnostic(StringRef DiagnosticName, 
+ const std::string &Message,
+ int FileOffset,
+ const std::string &FilePath,
+ const StringMap &Fix) {
+  DiagnosticMessage DiagMessage;
+  DiagMessage.Message = Message;
+  DiagMessage.FileOffset = FileOffset;
+  DiagMessage.FilePath = FilePath;
+  return Diagnostic(DiagnosticName, DiagMessage, Fix, {},
+Diagnostic::Warning, "path/to/build/directory");
+}
+
+TEST(DiagnosticsYamlTest, serializesDiagnostics) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+
+  StringMap Fix1 = { 
+{ 
+  "path/to/source.cpp",  
+  Replacements({ "path/to/source.cpp", 100, 12, "replacement #1" }) 
+} 
+  };
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55, 
+   "path/to/source.cpp", Fix1));
+  
+  StringMap Fix2 = {
+{ 
+  "path/to/header.h",
+  Replacements({ "path/t

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added a comment.

Thanks, Alex for your suggestions.
I agree with your remarks, but i thought it's only style preferences.
I'll fix it soon.


Repository:
  rL LLVM

https://reviews.llvm.org/D34404



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

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 105260.
vladimir.plyashkun added a comment.

- marked some arguments as `const`
- use `{}` instead of explicit variable declarations


Repository:
  rL LLVM

https://reviews.llvm.org/D34404

Files:
  include/clang/Tooling/DiagnosticsYaml.h
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/DiagnosticsYamlTest.cpp
  unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  unittests/clang-apply-replacements/CMakeLists.txt

Index: unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- /dev/null
+++ unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -0,0 +1,167 @@
+//===- unittests/Tooling/DiagnosticsYamlTest.cpp - Serialization tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Tests for serialization of Diagnostics.
+//
+//===--===//
+
+#include "clang/Tooling/Core/Diagnostic.h"
+#include "clang/Tooling/DiagnosticsYaml.h"
+#include "clang/Tooling/ReplacementsYaml.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang::tooling;
+
+static Diagnostic makeDiagnostic(StringRef DiagnosticName, 
+ const std::string &Message,
+ int FileOffset,
+ const std::string &FilePath,
+ const StringMap &Fix) {
+  DiagnosticMessage DiagMessage;
+  DiagMessage.Message = Message;
+  DiagMessage.FileOffset = FileOffset;
+  DiagMessage.FilePath = FilePath;
+  return Diagnostic(DiagnosticName, DiagMessage, Fix, {},
+Diagnostic::Warning, "path/to/build/directory");
+}
+
+TEST(DiagnosticsYamlTest, serializesDiagnostics) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+
+  StringMap Fix1{
+{ "path/to/source.cpp", Replacements(Replacement("path/to/source.cpp", 100,
+12, "replacement #1")) } };
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55, 
+   "path/to/source.cpp", Fix1));
+  
+  StringMap Fix2{
+{ "path/to/header.h",
+Replacements(Replacement("path/to/header.h", 62, 2, "replacement #2")) } };
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60, 
+   "path/to/header.h", Fix2));
+
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72, 
+   "path/to/source2.cpp", {}));
+
+  std::string YamlContent;
+  raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << TUD;
+
+  EXPECT_STREQ("---\n"
+"MainSourceFile:  path/to/source.cpp\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1\'\n"
+"Message: 'message #1'\n"
+"FileOffset:  55\n"
+"FilePath:path/to/source.cpp\n"
+"Replacements:\n"
+"  - FilePath:path/to/source.cpp\n"
+"Offset:  100\n"
+"Length:  12\n"
+"ReplacementText: 'replacement #1'\n"
+"  - DiagnosticName:  'diagnostic#2'\n"
+"Message: 'message #2'\n"
+"FileOffset:  60\n"
+"FilePath:path/to/header.h\n"
+"Replacements:\n"
+"  - FilePath:path/to/header.h\n"
+"Offset:  62\n"
+"Length:  2\n"
+"ReplacementText: 'replacement #2'\n"
+"  - DiagnosticName:  'diagnostic#3'\n"
+"Message: 'message #3'\n"
+"FileOffset:  72\n"
+"FilePath:path/to/source2.cpp\n"
+"Replacements:\n"
+"...\n",
+  YamlContentStream.str().c_str());
+}
+
+TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
+std::string YamlContent = "---\n"
+  "MainSourceFile:  path/to/source.cpp\n"
+  "Diagnostics: \n"
+  "  - DiagnosticName:  'diagnostic#1'\n"
+  "Message: 'message #1'\n"
+  "FileOffset:  55\n"
+  "FilePath:path/to/source.cpp\n"
+  "Replacements:\n"
+  "  - FilePath:path/to/source.cpp\n"
+  "Offset:  100\n"
+  "Length:  12\n"
+  "ReplacementText: 'replacement #1'\n"
+  "  - DiagnosticName:  'diagnostic#2'\n"
+  "Message: 'message #2'\n"
+  "FileOffset:  60\n"
+  "FilePath:path/to/header.h\n"
+  "Replacements:\n"
+  "  - FilePath:path/to/header.h\n"
+  "Offset:  62\n"
+  "Length:  2\n"
+  "Replacem

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added inline comments.



Comment at: unittests/clang-apply-replacements/ApplyReplacementsTest.cpp:23
+  SmallVector EmptyNotes;
+  return tooling::Diagnostic(DiagnosticName, Message, Replacements, EmptyNotes,
+ tooling::Diagnostic::Warning, BuildDirectory);

alexfh wrote:
> vladimir.plyashkun wrote:
> > alexfh wrote:
> > > alexfh wrote:
> > > > Will `{}` work instead of `EmptyNotes`?
> > > `tooling::` is not needed due to the using directive above. Same below.
> > Unfortunately, no.
> > Constructor expects non-const reference `SmallVector 
> > &Notes`, so i can't pass rvalue directly to it.
> I overlooked this when committing someone's patch. Fixed in r307143. Now you 
> can add `const` and probably also use `{}`.
Thanks, fixed it.


Repository:
  rL LLVM

https://reviews.llvm.org/D34404



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

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 105240.
vladimir.plyashkun added a comment.

- use `EXPECT_*` instead of `ASSERT_*` where it's possible
- `Diagnostic` constructor now takes `const` references for it's arguments
- removed extra namespace qualifiers


Repository:
  rL LLVM

https://reviews.llvm.org/D34404

Files:
  include/clang/Tooling/Core/Diagnostic.h
  include/clang/Tooling/DiagnosticsYaml.h
  lib/Tooling/Core/Diagnostic.cpp
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/DiagnosticsYamlTest.cpp
  unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  unittests/clang-apply-replacements/CMakeLists.txt

Index: unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- /dev/null
+++ unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -0,0 +1,168 @@
+//===- unittests/Tooling/DiagnosticsYamlTest.cpp - Serialization tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Tests for serialization of Diagnostics.
+//
+//===--===//
+
+#include "clang/Tooling/Core/Diagnostic.h"
+#include "clang/Tooling/DiagnosticsYaml.h"
+#include "clang/Tooling/ReplacementsYaml.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang::tooling;
+
+static Diagnostic makeDiagnostic(StringRef DiagnosticName, 
+ const std::string &Message,
+ int FileOffset,
+ const std::string &FilePath,
+ const StringMap &Fix) {
+  DiagnosticMessage DiagMessage;
+  DiagMessage.Message = Message;
+  DiagMessage.FileOffset = FileOffset;
+  DiagMessage.FilePath = FilePath;
+  return Diagnostic(DiagnosticName, DiagMessage, Fix, {},
+Diagnostic::Warning, "path/to/build/directory");
+}
+
+TEST(DiagnosticsYamlTest, serializesDiagnostics) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+
+  StringMap Fix1{
+{ "path/to/source.cpp", Replacements(Replacement("path/to/source.cpp", 100,
+12, "replacement #1")) } };
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55, 
+   "path/to/source.cpp", Fix1));
+  
+  StringMap Fix2{
+{ "path/to/header.h",
+Replacements(Replacement("path/to/header.h", 62, 2, "replacement #2")) } };
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60, 
+   "path/to/header.h", Fix2));
+
+  StringMap  Fix3;
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72, 
+   "path/to/source2.cpp", Fix3));
+
+  std::string YamlContent;
+  raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << TUD;
+
+  EXPECT_STREQ("---\n"
+"MainSourceFile:  path/to/source.cpp\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1\'\n"
+"Message: 'message #1'\n"
+"FileOffset:  55\n"
+"FilePath:path/to/source.cpp\n"
+"Replacements:\n"
+"  - FilePath:path/to/source.cpp\n"
+"Offset:  100\n"
+"Length:  12\n"
+"ReplacementText: 'replacement #1'\n"
+"  - DiagnosticName:  'diagnostic#2'\n"
+"Message: 'message #2'\n"
+"FileOffset:  60\n"
+"FilePath:path/to/header.h\n"
+"Replacements:\n"
+"  - FilePath:path/to/header.h\n"
+"Offset:  62\n"
+"Length:  2\n"
+"ReplacementText: 'replacement #2'\n"
+"  - DiagnosticName:  'diagnostic#3'\n"
+"Message: 'message #3'\n"
+"FileOffset:  72\n"
+"FilePath:path/to/source2.cpp\n"
+"Replacements:\n"
+"...\n",
+  YamlContentStream.str().c_str());
+}
+
+TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
+std::string YamlContent = "---\n"
+  "MainSourceFile:  path/to/source.cpp\n"
+  "Diagnostics: \n"
+  "  - DiagnosticName:  'diagnostic#1'\n"
+  "Message: 'message #1'\n"
+  "FileOffset:  55\n"
+  "FilePath:path/to/source.cpp\n"
+  "Replacements:\n"
+  "  - FilePath:path/to/source.cpp\n"
+  "Offset:  100\n"
+  "Length:  12\n"
+  "ReplacementText: 'replacement #1'\n"
+  "  - DiagnosticName:  'diagnostic#2'\n"
+  "Message: 'message #2'\n"
+  "FileOffset:  60\n"
+  "FilePath:path/to/header.h\n"
+ 

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added inline comments.



Comment at: unittests/clang-apply-replacements/ApplyReplacementsTest.cpp:18
+
+static tooling::Diagnostic makeDiagnostic(const StringRef DiagnosticName,
+  DiagnosticMessage &Message,

alexfh wrote:
> I'd remove `const` from `DiagnosticName`, since a StringRef doesn't allow to 
> modify the string it points to (and marking by value arguments `const` is not 
> common in LLVM). However, `Message` and `Replacements` allow to modify the 
> referenced objects and thus should be marked `const`.
> 
> Also consider inlining this method, it doesn't buy you much.
Same for here. 
Constructor for `Diagnostic` class requires non-const references for `Message` 
and `Replacements` arguments. So, it's invalid to mark them as `const`.


Repository:
  rL LLVM

https://reviews.llvm.org/D34404



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

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added inline comments.



Comment at: unittests/clang-apply-replacements/ApplyReplacementsTest.cpp:23
+  SmallVector EmptyNotes;
+  return tooling::Diagnostic(DiagnosticName, Message, Replacements, EmptyNotes,
+ tooling::Diagnostic::Warning, BuildDirectory);

alexfh wrote:
> alexfh wrote:
> > Will `{}` work instead of `EmptyNotes`?
> `tooling::` is not needed due to the using directive above. Same below.
Unfortunately, no.
Constructor expects non-const reference `SmallVector 
&Notes`, so i can't pass rvalue directly to it.


Repository:
  rL LLVM

https://reviews.llvm.org/D34404



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

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 105153.
vladimir.plyashkun added a subscriber: alexfh.
vladimir.plyashkun added a comment.

- moved test-case from separate file to existing one
- fixed `No newline at end of file` problem and put space inside comment


Repository:
  rL LLVM

https://reviews.llvm.org/D34440

Files:
  lib/Tooling/CommonOptionsParser.cpp
  unittests/Tooling/CompilationDatabaseTest.cpp

Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -10,12 +10,16 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclGroup.h"
 #include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/FileMatchTrie.h"
 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "gtest/gtest.h"
 
+#include 
+
 namespace clang {
 namespace tooling {
 
@@ -626,5 +630,52 @@
   EXPECT_EQ(2, Argc);
 }
 
+class ParseCompilationDatabaseFromResponseFileTest : public testing::Test {
+public:
+  SmallString<128> TestDir;
+  SmallString<128> ResponseFileName;
+
+  ParseCompilationDatabaseFromResponseFileTest() {
+std::error_code EC = llvm::sys::fs::createUniqueDirectory("unittest", TestDir);
+EXPECT_TRUE(!EC);
+llvm::sys::path::append(ResponseFileName, TestDir, "resp");
+  }
+  void setResponseFileContent(const std::string &content) {
+std::ofstream File(ResponseFileName.c_str());
+EXPECT_TRUE(File.is_open());
+File << content;
+File.close();
+  }
+  ~ParseCompilationDatabaseFromResponseFileTest() override {
+llvm::sys::fs::remove(ResponseFileName);
+llvm::sys::fs::remove(TestDir);
+  }
+};
+
+TEST_F(ParseCompilationDatabaseFromResponseFileTest, GetCompileCommands) {
+  setResponseFileContent("-- -a=true -b");
+  SmallString<128> ResponseRef;
+  ResponseRef.append(1, '@');
+  ResponseRef.append(ResponseFileName.c_str());
+  int Argc = 3;
+  const char *Argv[] = { "1", "2", ResponseRef.c_str() };
+  llvm::cl::OptionCategory Category("options");
+  llvm::cl::opt option1("a", llvm::cl::desc(""), llvm::cl::init(false),
+  llvm::cl::cat(Category));
+  llvm::cl::opt option2("b", llvm::cl::desc(""), llvm::cl::init(false),
+  llvm::cl::cat(Category));
+  CommonOptionsParser Parser(Argc, Argv, Category);
+  CompilationDatabase &Database = Parser.getCompilations();
+  std::vector ActualCC = Database.getCompileCommands("source");
+  ASSERT_EQ(1u, ActualCC.size());
+  std::vector ExpectedCmd;
+  ExpectedCmd.push_back("clang-tool");
+  ExpectedCmd.push_back("-a=true");
+  ExpectedCmd.push_back("-b");
+  ExpectedCmd.push_back("source");
+
+  ASSERT_EQ(ExpectedCmd, ActualCC[0].CommandLine);
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Tooling/CommonOptionsParser.cpp
===
--- lib/Tooling/CommonOptionsParser.cpp
+++ lib/Tooling/CommonOptionsParser.cpp
@@ -25,6 +25,7 @@
 //===--===//
 
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/StringSaver.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
@@ -115,6 +116,14 @@
   cl::cat(Category));
 
   cl::HideUnrelatedOptions(Category);
+  
+  // Expand response files before loading compilation database from command line
+  SmallVector newArgv(argv, argv + argc);
+  BumpPtrAllocator A;
+  StringSaver Saver(A);
+  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, newArgv);
+  argv = &newArgv[0];
+  argc = static_cast(newArgv.size());
 
   std::string ErrorMessage;
   Compilations =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 105078.
vladimir.plyashkun added a comment.
Herald added a subscriber: JDevlieghere.

- fixed `No newline at end of file` problem
- provided test-case to check that diagnostics with no fixes will be applied 
correctly


Repository:
  rL LLVM

https://reviews.llvm.org/D34404

Files:
  include/clang/Tooling/DiagnosticsYaml.h
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/DiagnosticsYamlTest.cpp
  unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  unittests/clang-apply-replacements/CMakeLists.txt

Index: unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- /dev/null
+++ unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -0,0 +1,183 @@
+//===- unittests/Tooling/DiagnosticsYamlTest.cpp - Serialization tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Tests for serialization of Diagnostics.
+//
+//===--===//
+
+#include "clang/Tooling/Core/Diagnostic.h"
+#include "clang/Tooling/DiagnosticsYaml.h"
+#include "clang/Tooling/ReplacementsYaml.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang::tooling;
+
+TEST(DiagnosticsYamlTest, SerializesDiagnostics) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+  DiagnosticMessage Message1;
+  Message1.Message = "message #1";
+  Message1.FileOffset = 55;
+  Message1.FilePath = "path/to/source.cpp";
+  StringMap Fix1{
+{ "path/to/source.cpp", Replacements(Replacement("path/to/source.cpp", 100,
+12, "replacement #1")) } };
+  DiagnosticMessage Message2;
+  Message2.Message = "message #2";
+  Message2.FileOffset = 60;
+  Message2.FilePath = "path/to/header.h";
+  StringMap Fix2{
+{ "path/to/header.h",
+Replacements(Replacement("path/to/header.h", 62, 2, "replacement #2")) } };
+  SmallVector EmptyNotes;
+  TUD.Diagnostics.emplace_back("diagnostic#1", Message1, Fix1, EmptyNotes,
+   Diagnostic::Warning, "path/to/build/directory");
+  TUD.Diagnostics.emplace_back("diagnostic#2", Message2, Fix2, EmptyNotes,
+   Diagnostic::Error, "path/to/another/build/directory");
+
+  std::string YamlContent;
+  raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << TUD;
+
+  ASSERT_STREQ("---\n"
+"MainSourceFile:  path/to/source.cpp\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1\'\n"
+"Message: 'message #1'\n"
+"FileOffset:  55\n"
+"FilePath:path/to/source.cpp\n"
+"Replacements:\n"
+"  - FilePath:path/to/source.cpp\n"
+"Offset:  100\n"
+"Length:  12\n"
+"ReplacementText: 'replacement #1'\n"
+"  - DiagnosticName:  'diagnostic#2'\n"
+"Message: 'message #2'\n"
+"FileOffset:  60\n"
+"FilePath:path/to/header.h\n"
+"Replacements:\n"
+"  - FilePath:path/to/header.h\n"
+"Offset:  62\n"
+"Length:  2\n"
+"ReplacementText: 'replacement #2'\n"
+"...\n",
+  YamlContentStream.str().c_str());
+}
+
+TEST(DiagnosticsYamlTest, SerializesDiagnosticWithNoFix) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+  DiagnosticMessage Message;
+  Message.Message = "message #1";
+  Message.FileOffset = 55;
+  Message.FilePath = "path/to/source.cpp";
+  StringMap EmptyReplacements;
+  SmallVector EmptyNotes;
+  TUD.Diagnostics.emplace_back("diagnostic#1", Message, EmptyReplacements,
+   EmptyNotes, Diagnostic::Warning,
+   "path/to/build/directory");
+
+  std::string YamlContent;
+  raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << TUD;
+
+  ASSERT_STREQ("---\n"
+"MainSourceFile:  path/to/source.cpp\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1'\n"
+"Message: 'message #1'\n"
+"FileOffset:  55\n"
+"FilePath:path/to/source.cpp\n"
+"Replacements:\n"
+"...\n",
+  YamlContentStream.str().c_str());
+}
+
+TEST(DiagnosticsYamlTest, DeserializesDiagnostics) {
+std::string YamlContent = "---\n"
+  "MainSourceFile:  path/to/source.cpp\n"
+  "Diagnostics: \n"
+  "  - DiagnosticName:  'diagnostic#1'\n"
+  "Message: 'message #1'\n"
+  "FileOffset:  55\n"
+  "FilePath:path/to/source.cpp\n"
+  "Replacements:\n"
+  "  - FilePath:pat

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 103353.
vladimir.plyashkun added a comment.

correct revision with all changes


Repository:
  rL LLVM

https://reviews.llvm.org/D34404

Files:
  include/clang/Tooling/DiagnosticsYaml.h
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/DiagnosticsYamlTest.cpp

Index: unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- /dev/null
+++ unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -0,0 +1,170 @@
+//===- unittests/Tooling/DiagnosticsYamlTest.cpp - Serialization tests ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Tests for serialization of Diagnostics.
+//
+//===--===//
+
+#include "clang/Tooling/Core/Diagnostic.h"
+#include "clang/Tooling/DiagnosticsYaml.h"
+#include "clang/Tooling/ReplacementsYaml.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang::tooling;
+
+TEST(DiagnosticsYamlTest, SerializesDiagnostics) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+  DiagnosticMessage Message1;
+  Message1.Message = "message #1";
+  Message1.FileOffset = 55;
+  Message1.FilePath = "path/to/source.cpp";
+  StringMap Fix1{
+  {"path/to/source.cpp", Replacements(Replacement("path/to/source.cpp", 100,
+  12, "replacement #1"))}};
+  DiagnosticMessage Message2;
+  Message2.Message = "message #2";
+  Message2.FileOffset = 60;
+  Message2.FilePath = "path/to/header.h";
+  StringMap Fix2{
+  {"path/to/header.h",
+   Replacements(Replacement("path/to/header.h", 62, 2, "replacement #2"))}};
+  SmallVector EmptyNotes;
+  TUD.Diagnostics.emplace_back("diagnostic#1", Message1, Fix1, EmptyNotes,
+   Diagnostic::Warning, "path/to/build/directory");
+  TUD.Diagnostics.emplace_back("diagnostic#2", Message2, Fix2, EmptyNotes,
+   Diagnostic::Error,
+   "path/to/another/build/directory");
+
+  std::string YamlContent;
+  raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << TUD;
+
+  ASSERT_STREQ("---\n"
+   "MainSourceFile:  path/to/source.cpp\n"
+   "Diagnostics: \n"
+   "  - DiagnosticName:  \"diagnostic#1\"\n"
+   "Message: \"message #1\"\n"
+   "FileOffset:  55\n"
+   "FilePath:path/to/source.cpp\n"
+   "Replacements:\n"
+   "  - FilePath:path/to/source.cpp\n"
+   "Offset:  100\n"
+   "Length:  12\n"
+   "ReplacementText: \"replacement #1\"\n"
+   "  - DiagnosticName:  \"diagnostic#2\"\n"
+   "Message: \"message #2\"\n"
+   "FileOffset:  60\n"
+   "FilePath:path/to/header.h\n"
+   "Replacements:\n"
+   "  - FilePath:path/to/header.h\n"
+   "Offset:  62\n"
+   "Length:  2\n"
+   "ReplacementText: \"replacement #2\"\n"
+   "...\n",
+   YamlContentStream.str().c_str());
+}
+
+TEST(DiagnosticsYamlTest, SerializesDiagnosticWithNoFix) {
+  TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = "path/to/source.cpp";
+  DiagnosticMessage Message;
+  Message.Message = "message #1";
+  Message.FileOffset = 55;
+  Message.FilePath = "path/to/source.cpp";
+  StringMap EmptyReplacements;
+  SmallVector EmptyNotes;
+  TUD.Diagnostics.emplace_back("diagnostic#1", Message, EmptyReplacements,
+   EmptyNotes, Diagnostic::Warning,
+   "path/to/build/directory");
+
+  std::string YamlContent;
+  raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << TUD;
+
+  ASSERT_STREQ("---\n"
+   "MainSourceFile:  path/to/source.cpp\n"
+   "Diagnostics: \n"
+   "  - DiagnosticName:  \"diagnostic#1\"\n"
+   "Message: \"message #1\"\n"
+   "FileOffset:  55\n"
+   "FilePath:path/to/source.cpp\n"
+   "...\n",
+   YamlContentStream.str().c_str());
+}
+
+TEST(DiagnosticsYamlTest, DeserializesDiagnostics) {
+  std::string YamlContent = "---\n"
+"MainSourceFile:  path/to/source.cpp\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1'\

[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun updated this revision to Diff 103341.
vladimir.plyashkun added a comment.
Herald added subscribers: xazax.hun, mgorny.

updated CMakeLists.txt


Repository:
  rL LLVM

https://reviews.llvm.org/D34404

Files:
  unittests/Tooling/CMakeLists.txt


Index: unittests/Tooling/CMakeLists.txt
===
--- unittests/Tooling/CMakeLists.txt
+++ unittests/Tooling/CMakeLists.txt
@@ -13,6 +13,7 @@
 add_clang_unittest(ToolingTests
   CommentHandlerTest.cpp
   CompilationDatabaseTest.cpp
+  DiagnosticsYamlTest.cpp
   FixItTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.cpp


Index: unittests/Tooling/CMakeLists.txt
===
--- unittests/Tooling/CMakeLists.txt
+++ unittests/Tooling/CMakeLists.txt
@@ -13,6 +13,7 @@
 add_clang_unittest(ToolingTests
   CommentHandlerTest.cpp
   CompilationDatabaseTest.cpp
+  DiagnosticsYamlTest.cpp
   FixItTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.cpp
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34440: [Clang] Expand response files before loading compilation database

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun created this revision.
vladimir.plyashkun added a project: clang.
Herald added a subscriber: mgorny.

Due to command line length restrictions, arguments can be passed through 
response files. 
Before trying to load compilation database from command line, response files 
should be expanded first.


Repository:
  rL LLVM

https://reviews.llvm.org/D34440

Files:
  lib/Tooling/CommonOptionsParser.cpp
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/CommonOptionsParserTest.cpp

Index: unittests/Tooling/CMakeLists.txt
===
--- unittests/Tooling/CMakeLists.txt
+++ unittests/Tooling/CMakeLists.txt
@@ -12,6 +12,7 @@
 
 add_clang_unittest(ToolingTests
   CommentHandlerTest.cpp
+  CommonOptionsParserTest.cpp
   CompilationDatabaseTest.cpp
   FixItTest.cpp
   LookupTest.cpp
Index: unittests/Tooling/CommonOptionsParserTest.cpp
===
--- /dev/null
+++ unittests/Tooling/CommonOptionsParserTest.cpp
@@ -0,0 +1,68 @@
+//===- unittests/Tooling/CommonOptionsParserTest.cpp
+//--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace llvm;
+using namespace clang::tooling;
+
+class CommonOptionsParserTest : public testing::Test {
+public:
+  SmallString<128> TestDir;
+  SmallString<128> ResponseFileName;
+
+  CommonOptionsParserTest() {
+std::error_code EC = sys::fs::createUniqueDirectory("unittest", TestDir);
+EXPECT_TRUE(!EC);
+sys::path::append(ResponseFileName, TestDir, "resp");
+  }
+  void setResponseFileContent(const std::string &content) {
+std::ofstream File(ResponseFileName.c_str());
+EXPECT_TRUE(File.is_open());
+File << content;
+File.close();
+  }
+  ~CommonOptionsParserTest() override {
+sys::fs::remove(ResponseFileName);
+sys::fs::remove(TestDir);
+  }
+};
+
+TEST_F(CommonOptionsParserTest,
+   ExpandResponseFileBeforeLoadingCompilationDatabase) {
+  setResponseFileContent("-option_1 -- -option_2=true -option_3");
+  SmallString<128> ResponseRef;
+  ResponseRef.append(1, '@');
+  ResponseRef.append(ResponseFileName.c_str());
+  int Argc = 3;
+  const char *Argv[] = {"pos_1", "pos_2", ResponseRef.c_str()};
+  cl::OptionCategory Category("options");
+  cl::opt option1("option_1", cl::desc(""), cl::init(false),
+cl::cat(Category));
+  cl::opt option2("option_2", cl::desc(""), cl::init(false),
+cl::cat(Category));
+  cl::opt option3("option_3", cl::desc(""), cl::init(false),
+cl::cat(Category));
+  CommonOptionsParser Parser(Argc, Argv, Category);
+  CompilationDatabase &Database = Parser.getCompilations();
+  std::vector ActualCC = Database.getCompileCommands("source");
+  ASSERT_EQ(1u, ActualCC.size());
+  std::vector ExpectedCmd;
+  ExpectedCmd.push_back("clang-tool");
+  ExpectedCmd.push_back("-option_2=true");
+  ExpectedCmd.push_back("-option_3");
+  ExpectedCmd.push_back("source");
+
+  ASSERT_EQ(ExpectedCmd, ActualCC[0].CommandLine);
+}
\ No newline at end of file
Index: lib/Tooling/CommonOptionsParser.cpp
===
--- lib/Tooling/CommonOptionsParser.cpp
+++ lib/Tooling/CommonOptionsParser.cpp
@@ -25,6 +25,7 @@
 //===--===//
 
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/StringSaver.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
@@ -115,7 +116,13 @@
   cl::cat(Category));
 
   cl::HideUnrelatedOptions(Category);
-
+  //Expand response files before loading compilation database from command line
+  SmallVector newArgv(argv, argv + argc);
+  BumpPtrAllocator A;
+  StringSaver Saver(A);
+  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, newArgv);
+  argv = &newArgv[0];
+  argc = static_cast(newArgv.size());
   std::string ErrorMessage;
   Compilations =
   FixedCompilationDatabase::loadFromCommandLine(argc, argv, ErrorMessage);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits