[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

Thanks! The change looks good now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 194817.
hokein marked an inline comment as done.
hokein added a comment.

Update and rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/add_new_check.py
  clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
  clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file2.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file2.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file3.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/format/no.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/format/yes.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file2.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file2.yaml
  clang-tools-extra/test/clang-tidy/export-diagnostics.cpp
  clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
  clang/include/clang/Tooling/Core/Diagnostic.h
  clang/include/clang/Tooling/DiagnosticsYaml.h
  clang/lib/Tooling/Core/Diagnostic.cpp
  clang/unittests/Tooling/DiagnosticsYamlTest.cpp

Index: clang/unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- clang/unittests/Tooling/DiagnosticsYamlTest.cpp
+++ clang/unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -20,11 +20,13 @@
 using clang::tooling::Diagnostic;
 
 static DiagnosticMessage makeMessage(const std::string , int FileOffset,
- const std::string ) {
+ const std::string ,
+ const StringMap ) {
   DiagnosticMessage DiagMessage;
   DiagMessage.Message = Message;
   DiagMessage.FileOffset = FileOffset;
   DiagMessage.FilePath = FilePath;
+  DiagMessage.Fix = Fix;
   return DiagMessage;
 }
 
@@ -32,10 +34,52 @@
  const std::string , int FileOffset,
  const std::string ,
  const StringMap ) {
-  return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath),
-Fix, {}, Diagnostic::Warning, "path/to/build/directory");
+  return Diagnostic(DiagnosticName,
+makeMessage(Message, FileOffset, FilePath, Fix), {},
+Diagnostic::Warning, "path/to/build/directory");
 }
 
+static const char *YAMLContent =
+"---\n"
+"MainSourceFile:  'path/to/source.cpp'\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1\'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #1'\n"
+"  FilePath:'path/to/source.cpp'\n"
+"  FileOffset:  55\n"
+"  Replacements:\n"
+"- FilePath:'path/to/source.cpp'\n"
+"  Offset:  100\n"
+"  Length:  12\n"
+"  ReplacementText: 'replacement #1'\n"
+"  - DiagnosticName:  'diagnostic#2'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #2'\n"
+"  FilePath:'path/to/header.h'\n"
+"  FileOffset:  60\n"
+"  Replacements:\n"
+"- FilePath:'path/to/header.h'\n"
+"  Offset:  62\n"
+"  Length:  2\n"
+"  ReplacementText: 'replacement #2'\n"
+"  - DiagnosticName:  'diagnostic#3'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #3'\n"
+"  FilePath:'path/to/source2.cpp'\n"
+"  FileOffset:  72\n"
+"  Replacements:[]\n"
+"Notes:   \n"
+"  - Message: Note1\n"
+"FilePath:'path/to/note1.cpp'\n"
+"FileOffset:  88\n"
+"Replacements:[]\n"
+"  - Message: Note2\n"
+"FilePath:'path/to/note2.cpp'\n"
+"FileOffset:  99\n"
+"Replacements:[]\n"
+"...\n";
+
 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
   TranslationUnitDiagnostics TUD;
   TUD.MainSourceFile = "path/to/source.cpp";
@@ -55,9 +99,9 @@
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
 

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/include/clang/Tooling/Core/Diagnostic.h:67-71
+  // Get the chosen fix to apply for this diagnostic.
+  // FIXME: currently we choose the first non-empty fix, extend it to support
+  // fix selection.
+  const llvm::StringMap *getChosenFix() const;
+

hokein wrote:
> alexfh wrote:
> > hokein wrote:
> > > alexfh wrote:
> > > > Do we actually need this method here? This whole structure is sort of a 
> > > > data-only serialization helper, and this method is adding some 
> > > > (arbitrary) logic into it.
> > > We have a few places using this method, or we could move this method out 
> > > of this structure?
> > I'd move it out closer to the code which uses it and call it 
> > "selectFirstFix", for example (the name should be a verb and it should be 
> > less vague about what the function is doing).
> hmm, this function is used in `ApplyReplacements.cpp`, `ClangTidy.cpp`, 
> `ClangTidyTest.h`, `ClangTidyDiagnosticConsumer.cpp`.
> 
> `Diagnostic.h` seems the most suitable place.
moved it out of the structure; 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added inline comments.



Comment at: clang/include/clang/Tooling/Core/Diagnostic.h:67-71
+  // Get the chosen fix to apply for this diagnostic.
+  // FIXME: currently we choose the first non-empty fix, extend it to support
+  // fix selection.
+  const llvm::StringMap *getChosenFix() const;
+

alexfh wrote:
> hokein wrote:
> > alexfh wrote:
> > > Do we actually need this method here? This whole structure is sort of a 
> > > data-only serialization helper, and this method is adding some 
> > > (arbitrary) logic into it.
> > We have a few places using this method, or we could move this method out of 
> > this structure?
> I'd move it out closer to the code which uses it and call it 
> "selectFirstFix", for example (the name should be a verb and it should be 
> less vague about what the function is doing).
hmm, this function is used in `ApplyReplacements.cpp`, `ClangTidy.cpp`, 
`ClangTidyTest.h`, `ClangTidyDiagnosticConsumer.cpp`.

`Diagnostic.h` seems the most suitable place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang/include/clang/Tooling/Core/Diagnostic.h:67-71
+  // Get the chosen fix to apply for this diagnostic.
+  // FIXME: currently we choose the first non-empty fix, extend it to support
+  // fix selection.
+  const llvm::StringMap *getChosenFix() const;
+

hokein wrote:
> alexfh wrote:
> > Do we actually need this method here? This whole structure is sort of a 
> > data-only serialization helper, and this method is adding some (arbitrary) 
> > logic into it.
> We have a few places using this method, or we could move this method out of 
> this structure?
I'd move it out closer to the code which uses it and call it "selectFirstFix", 
for example (the name should be a verb and it should be less vague about what 
the function is doing).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/include/clang/Tooling/Core/Diagnostic.h:67-71
+  // Get the chosen fix to apply for this diagnostic.
+  // FIXME: currently we choose the first non-empty fix, extend it to support
+  // fix selection.
+  const llvm::StringMap *getChosenFix() const;
+

alexfh wrote:
> Do we actually need this method here? This whole structure is sort of a 
> data-only serialization helper, and this method is adding some (arbitrary) 
> logic into it.
We have a few places using this method, or we could move this method out of 
this structure?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang/include/clang/Tooling/Core/Diagnostic.h:67-71
+  // Get the chosen fix to apply for this diagnostic.
+  // FIXME: currently we choose the first non-empty fix, extend it to support
+  // fix selection.
+  const llvm::StringMap *getChosenFix() const;
+

Do we actually need this method here? This whole structure is sort of a 
data-only serialization helper, and this method is adding some (arbitrary) 
logic into it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 193857.
hokein added a comment.

Remove a stale comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/add_new_check.py
  clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
  clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file2.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file2.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file3.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/format/no.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/format/yes.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file2.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file2.yaml
  clang-tools-extra/test/clang-tidy/export-diagnostics.cpp
  clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
  clang/include/clang/Tooling/Core/Diagnostic.h
  clang/include/clang/Tooling/DiagnosticsYaml.h
  clang/lib/Tooling/Core/Diagnostic.cpp
  clang/unittests/Tooling/DiagnosticsYamlTest.cpp

Index: clang/unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- clang/unittests/Tooling/DiagnosticsYamlTest.cpp
+++ clang/unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -20,11 +20,13 @@
 using clang::tooling::Diagnostic;
 
 static DiagnosticMessage makeMessage(const std::string , int FileOffset,
- const std::string ) {
+ const std::string ,
+ const StringMap ) {
   DiagnosticMessage DiagMessage;
   DiagMessage.Message = Message;
   DiagMessage.FileOffset = FileOffset;
   DiagMessage.FilePath = FilePath;
+  DiagMessage.Fix = Fix;
   return DiagMessage;
 }
 
@@ -32,10 +34,52 @@
  const std::string , int FileOffset,
  const std::string ,
  const StringMap ) {
-  return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath),
-Fix, {}, Diagnostic::Warning, "path/to/build/directory");
+  return Diagnostic(DiagnosticName,
+makeMessage(Message, FileOffset, FilePath, Fix), {},
+Diagnostic::Warning, "path/to/build/directory");
 }
 
+static const char *YAMLContent =
+"---\n"
+"MainSourceFile:  'path/to/source.cpp'\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1\'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #1'\n"
+"  FilePath:'path/to/source.cpp'\n"
+"  FileOffset:  55\n"
+"  Replacements:\n"
+"- FilePath:'path/to/source.cpp'\n"
+"  Offset:  100\n"
+"  Length:  12\n"
+"  ReplacementText: 'replacement #1'\n"
+"  - DiagnosticName:  'diagnostic#2'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #2'\n"
+"  FilePath:'path/to/header.h'\n"
+"  FileOffset:  60\n"
+"  Replacements:\n"
+"- FilePath:'path/to/header.h'\n"
+"  Offset:  62\n"
+"  Length:  2\n"
+"  ReplacementText: 'replacement #2'\n"
+"  - DiagnosticName:  'diagnostic#3'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #3'\n"
+"  FilePath:'path/to/source2.cpp'\n"
+"  FileOffset:  72\n"
+"  Replacements:[]\n"
+"Notes:   \n"
+"  - Message: Note1\n"
+"FilePath:'path/to/note1.cpp'\n"
+"FileOffset:  88\n"
+"Replacements:[]\n"
+"  - Message: Note2\n"
+"FilePath:'path/to/note2.cpp'\n"
+"FileOffset:  99\n"
+"Replacements:[]\n"
+"...\n";
+
 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
   TranslationUnitDiagnostics TUD;
   TUD.MainSourceFile = "path/to/source.cpp";
@@ -55,9 +99,9 @@
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
 

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D59932#1453565 , @alexfh wrote:

> This looks like a more promising direction. Thanks for the readiness to 
> experiment with this.
>
> See the comments inline.


Thanks for the comments. Now all existing tests are passed, the patch is in a 
good shape for review.

There is one missing point -- we don't test the fix-description notes in the 
lit tests, the current test mechanism (CHECK-MESSAGE, CHECK-NOTES) doesn't 
handle it well, we need to feature it out. I think this can be done in a 
separate patch.




Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:130
+
+  // If we have alternative fixes (attached via diagnostic::Notes), we just
+  // choose the first one to apply.

alexfh wrote:
> Could you leave a FIXME here to explore options around interactive fix 
> selection?
Done. I moved this code to `Diagnostic::getChosenFix()` as we have a few places 
using it.



Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:144
+
+  if (ApplyFixes && ErrorFix) {
+for (const auto  : *ErrorFix) {

alexfh wrote:
> The nesting level starts getting out of control here. I'd try to pull the 
> loop into a function.
Agree, but making such refactoring change in this patch will add noise to the 
diff, I tend to make the change in this patch as mini as possible.

I think this can be improved in a follow-up change. 



Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:187
   }
+  reportFix(Diag, Error.Message.Fix);
 }

alexfh wrote:
> Why `Error.Message.Fix`? Should this use `*SelectedFix` instead?
I think we still want to report all the alternative fixes to clients to align 
with clang's behavior. We only use the `SelectedFix` when applying the fix. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 193856.
hokein marked 8 inline comments as done.
hokein added a comment.

Fix apply-replacements, and address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/add_new_check.py
  clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
  clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file2.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file2.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file3.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/format/no.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/format/yes.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file2.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file2.yaml
  clang-tools-extra/test/clang-tidy/export-diagnostics.cpp
  clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
  clang/include/clang/Tooling/Core/Diagnostic.h
  clang/include/clang/Tooling/DiagnosticsYaml.h
  clang/lib/Tooling/Core/Diagnostic.cpp
  clang/unittests/Tooling/DiagnosticsYamlTest.cpp

Index: clang/unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- clang/unittests/Tooling/DiagnosticsYamlTest.cpp
+++ clang/unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -20,11 +20,13 @@
 using clang::tooling::Diagnostic;
 
 static DiagnosticMessage makeMessage(const std::string , int FileOffset,
- const std::string ) {
+ const std::string ,
+ const StringMap ) {
   DiagnosticMessage DiagMessage;
   DiagMessage.Message = Message;
   DiagMessage.FileOffset = FileOffset;
   DiagMessage.FilePath = FilePath;
+  DiagMessage.Fix = Fix;
   return DiagMessage;
 }
 
@@ -32,10 +34,52 @@
  const std::string , int FileOffset,
  const std::string ,
  const StringMap ) {
-  return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath),
-Fix, {}, Diagnostic::Warning, "path/to/build/directory");
+  return Diagnostic(DiagnosticName,
+makeMessage(Message, FileOffset, FilePath, Fix), {},
+Diagnostic::Warning, "path/to/build/directory");
 }
 
+static const char *YAMLContent =
+"---\n"
+"MainSourceFile:  'path/to/source.cpp'\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1\'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #1'\n"
+"  FilePath:'path/to/source.cpp'\n"
+"  FileOffset:  55\n"
+"  Replacements:\n"
+"- FilePath:'path/to/source.cpp'\n"
+"  Offset:  100\n"
+"  Length:  12\n"
+"  ReplacementText: 'replacement #1'\n"
+"  - DiagnosticName:  'diagnostic#2'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #2'\n"
+"  FilePath:'path/to/header.h'\n"
+"  FileOffset:  60\n"
+"  Replacements:\n"
+"- FilePath:'path/to/header.h'\n"
+"  Offset:  62\n"
+"  Length:  2\n"
+"  ReplacementText: 'replacement #2'\n"
+"  - DiagnosticName:  'diagnostic#3'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #3'\n"
+"  FilePath:'path/to/source2.cpp'\n"
+"  FileOffset:  72\n"
+"  Replacements:[]\n"
+"Notes:   \n"
+"  - Message: Note1\n"
+"FilePath:'path/to/note1.cpp'\n"
+"FileOffset:  88\n"
+"Replacements:[]\n"
+"  - Message: Note2\n"
+"FilePath:'path/to/note2.cpp'\n"
+"FileOffset:  99\n"
+"Replacements:[]\n"
+"...\n";
+
 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
   TranslationUnitDiagnostics TUD;
   TUD.MainSourceFile = "path/to/source.cpp";
@@ -55,9 +99,9 @@
   

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

This looks like a more promising direction. Thanks for the readiness to 
experiment with this.

See the comments inline.




Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:130
+
+  // If we have alternative fixes (attached via diagnostic::Notes), we just
+  // choose the first one to apply.

Could you leave a FIXME here to explore options around interactive fix 
selection?



Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:132
+  // choose the first one to apply.
+  const llvm::StringMap *ErrorFix = nullptr;
+  if (!Error.Message.Fix.empty())

`ErrorFix` brings more questions than it answers. Maybe `SelectedFix`, 
`ChosenFix`, or just `Fix`?



Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:133
+  const llvm::StringMap *ErrorFix = nullptr;
+  if (!Error.Message.Fix.empty())
+ErrorFix = 

nit: I'd add braces here, since the `else` branch has them.



Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:144
+
+  if (ApplyFixes && ErrorFix) {
+for (const auto  : *ErrorFix) {

The nesting level starts getting out of control here. I'd try to pull the loop 
into a function.



Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:187
   }
+  reportFix(Diag, Error.Message.Fix);
 }

Why `Error.Message.Fix`? Should this use `*SelectedFix` instead?



Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:262
+  for (const auto  : FileAndReplacements.second) {
+if (Repl.isApplicable()) {
+  SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();

nit: Early "continue" would make sense here.

  if (!Repl.isApplicable())
continue;
  ...



Comment at: clang/include/clang/Tooling/Core/Diagnostic.h:46
+
+  /// Fixes for this diagnostic.
+  llvm::StringMap Fix;

Some information from the original comment was lost here:

  /// Fixes to apply, grouped by file path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Running out of time today, the patch is not finished yet, but it should be good 
for another initial review/comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 193510.
hokein added a comment.

Emit the check fix description via diagnostic::note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
  clang-tools-extra/test/clang-tidy/export-diagnostics.cpp
  clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
  clang/include/clang/Tooling/Core/Diagnostic.h
  clang/include/clang/Tooling/DiagnosticsYaml.h
  clang/lib/Tooling/Core/Diagnostic.cpp
  clang/unittests/Tooling/DiagnosticsYamlTest.cpp

Index: clang/unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- clang/unittests/Tooling/DiagnosticsYamlTest.cpp
+++ clang/unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -20,11 +20,13 @@
 using clang::tooling::Diagnostic;
 
 static DiagnosticMessage makeMessage(const std::string , int FileOffset,
- const std::string ) {
+ const std::string ,
+ const StringMap ) {
   DiagnosticMessage DiagMessage;
   DiagMessage.Message = Message;
   DiagMessage.FileOffset = FileOffset;
   DiagMessage.FilePath = FilePath;
+  DiagMessage.Fix = Fix;
   return DiagMessage;
 }
 
@@ -32,10 +34,52 @@
  const std::string , int FileOffset,
  const std::string ,
  const StringMap ) {
-  return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath),
-Fix, {}, Diagnostic::Warning, "path/to/build/directory");
+  return Diagnostic(DiagnosticName,
+makeMessage(Message, FileOffset, FilePath, Fix), {},
+Diagnostic::Warning, "path/to/build/directory");
 }
 
+static const char *YAMLContent =
+"---\n"
+"MainSourceFile:  'path/to/source.cpp'\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1\'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #1'\n"
+"  FilePath:'path/to/source.cpp'\n"
+"  FileOffset:  55\n"
+"  Replacements:\n"
+"- FilePath:'path/to/source.cpp'\n"
+"  Offset:  100\n"
+"  Length:  12\n"
+"  ReplacementText: 'replacement #1'\n"
+"  - DiagnosticName:  'diagnostic#2'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #2'\n"
+"  FilePath:'path/to/header.h'\n"
+"  FileOffset:  60\n"
+"  Replacements:\n"
+"- FilePath:'path/to/header.h'\n"
+"  Offset:  62\n"
+"  Length:  2\n"
+"  ReplacementText: 'replacement #2'\n"
+"  - DiagnosticName:  'diagnostic#3'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #3'\n"
+"  FilePath:'path/to/source2.cpp'\n"
+"  FileOffset:  72\n"
+"  Replacements:[]\n"
+"Notes:   \n"
+"  - Message: Note1\n"
+"FilePath:'path/to/note1.cpp'\n"
+"FileOffset:  88\n"
+"Replacements:[]\n"
+"  - Message: Note2\n"
+"FilePath:'path/to/note2.cpp'\n"
+"FileOffset:  99\n"
+"Replacements:[]\n"
+"...\n";
+
 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
   TranslationUnitDiagnostics TUD;
   TUD.MainSourceFile = "path/to/source.cpp";
@@ -55,9 +99,9 @@
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
"path/to/source2.cpp", {}));
   TUD.Diagnostics.back().Notes.push_back(
-  makeMessage("Note1", 88, "path/to/note1.cpp"));
+  makeMessage("Note1", 88, "path/to/note1.cpp", {}));
   TUD.Diagnostics.back().Notes.push_back(
-  makeMessage("Note2", 99, "path/to/note2.cpp"));
+  makeMessage("Note2", 99, "path/to/note2.cpp", {}));
 
   std::string YamlContent;
   raw_string_ostream YamlContentStream(YamlContent);
@@ -65,80 +109,12 @@
   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"
- 

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added a comment.

As discussed offline, the current approach only works for checks provide a 
single fix, providing such API is somehow misleading.

Instead, we'd emit the check fix and the fix description via diagnostic::Note, 
rather than attaching to the main diagnostic of the check:

- match the data model of the checks provide different semantic fixes depending 
on the context;
- align with the way how existing clang diagnostics emit alternative fixes (via 
diagnostic::Note);
- open a door to write clang-tidy checks that provide alternative fixes; we 
don't have these checks at the moment, but some clang diagnostics like 
`clang-diagnostic-parentheses` do (and our current implementation just 
aggregates all the fixes together, which is not correct);

It would require some changes in clang-tidy check side:

Before this patch:

  void MyCheck::check(...) {
 ...
 diag(loc, "my check warning") <<  FixtItHint::CreateReplacement(...);
  }

After this patch:

  void MyCheck::check(...) {
 ...
 diag(loc, "my check warning"); // Emit a check warning
 // We might want to introduce an utility method like `diagFix` to save 
some verbosed words.
 diag(loc, "fix description", DiagnosticIDs::Note) << 
FixtItHint::CreateReplacement(...); // Emit a diagnostic note and a fix
  }

An example of unused-using-decls check (clang-tidy command line output)

- before:

  1 warning generated.
  /tmp/test.cpp:8:12: warning: using decl 'Foo' is unused 
[misc-unused-using-decls]
  using foo::Foo;
  ~~~^~~~

- after:

  1 warning generated.
  /tmp/test.cpp:8:12: warning: using decl 'Foo' is unused 
[misc-unused-using-decls]
  using foo::Foo;
 ^
  /tmp/test.cpp:8:12: note: remove the using
  using foo::Foo;
  ~~~^~~~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:109
+  /// not supported.
+  virtual llvm::StringRef fixDescription() { return ""; }
+

alexfh wrote:
> Checks can provide different fixes with different semantics depending on the 
> context. It seems incorrect to assume that there can be one reasonable 
> description for all of them.
Yes, as mentioned in the comment, this is the limitation of this approach -- I 
assume we don't have a large number of these checks, most checks are providing 
a  single fix.

Another alternative is to use some heuristic mechanisms to do a translation 
from the `Replacement`, like we do in clangd, 
https://github.com/llvm-mirror/clang-tools-extra/blob/master/clangd/Diagnostics.cpp#L350.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:75
   Context.diag(CheckName, PD->getLocation().asLocation(),
-   PD->getShortDescription())
+   PD->getShortDescription(), /*FixDescription*/ "")
   << PD->path.back()->getRanges();

nit: Use the format that the bugprone-argument-comment check understands: 
`/*FixDescription=*/`

Same below.



Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:109
+  /// not supported.
+  virtual llvm::StringRef fixDescription() { return ""; }
+

Checks can provide different fixes with different semantics depending on the 
context. It seems incorrect to assume that there can be one reasonable 
description for all of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D59932#1446533 , @JonasToth wrote:

> I think the idea is good and implementation, too. If we iterate all checks 
> anyway (probably?) we could think about adding a severity to the checks, too?


In a similar vein, I often feel like I'd like to be able to define a AllowFixIt 
on every checker, so I could turn off the FixIts for things we just want to 
warn on.

I often run clang-tidy with `-fix` but I don't want to necessarily want 
clang-tidy to fix everything I have marked as -check. 
readability-identifier-naming is one such checker that isn't mature enough to 
get everything correct, I want to see the warning to prevent new violations 
from creeping in, but don't actually want it to do a fixit.

However I don't want to have to keep commenting the check out of my .clang-tidy 
file just so I can run with -fix, it would be great if we could add   
*.AllowFixIt

i.e.

  - key: -naming.AllowFixIt
value:   'true/false'

To allow us to disable FixIts for certain checkers

  - key: readability-identifier-naming.AllowFixIt
value:   'false'

Just and idea, sorry for hijacking the thread.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D59932#1446533 , @JonasToth wrote:

> I think the idea is good and implementation, too. If we iterate all checks 
> anyway (probably?) we could think about adding a severity to the checks, too?
>
> I know that code-checker and code-compass have something like this to signal 
> importance of problems (say bugprone and cert differ from readability for 
> example).


Thanks. Unfortunately, we have to iterate all checks no matter which solution 
we use ;(

Adding a severity to checks is an interesting idea,  we need to define the 
semantic of the severity, looks like different analyzers define them in 
different ways (some defined by check quality, like stable enough/production 
ready; some defined by importance of the problem that the check find). And the 
existing modules of clang-tidy checks address this problem in some degree (like 
bugprone, readability modules).  I think it is a separate topic that needs more 
thoughts and discussions.




Comment at: clang-tidy/ClangTidyCheck.h:109
+  /// not supported.
+  virtual llvm::StringRef fixDescription() { return ""; }
+

Eugene.Zelenko wrote:
> return {} could be used instead.
yes, `{}` works here, but I'd use `""` which is more explicit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 193070.
hokein marked 2 inline comments as done.
hokein added a comment.

Update the patch:

- move FixDescriptions to tooling diagnostics, YAML serialization support
- add tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/ClangTidyCheck.h
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/test/clang-tidy/export-diagnostics.cpp
  clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp
  clang/include/clang/Tooling/Core/Diagnostic.h
  clang/include/clang/Tooling/DiagnosticsYaml.h
  clang/lib/Tooling/Core/Diagnostic.cpp
  clang/unittests/Tooling/DiagnosticsYamlTest.cpp

Index: clang/unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- clang/unittests/Tooling/DiagnosticsYamlTest.cpp
+++ clang/unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -31,9 +31,11 @@
 static Diagnostic makeDiagnostic(StringRef DiagnosticName,
  const std::string , int FileOffset,
  const std::string ,
- const StringMap ) {
+ const StringMap ,
+ const std::string ) {
   return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath),
-Fix, {}, Diagnostic::Warning, "path/to/build/directory");
+Fix, {}, Diagnostic::Warning, "path/to/build/directory",
+FixDescription);
 }
 
 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
@@ -44,16 +46,18 @@
   {"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));
+   "path/to/source.cpp", Fix1,
+   "fix1 description"));
 
   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));
+   "path/to/header.h", Fix2,
+   "fix2 description"));
 
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
-   "path/to/source2.cpp", {}));
+   "path/to/source2.cpp", {}, ""));
   TUD.Diagnostics.back().Notes.push_back(
   makeMessage("Note1", 88, "path/to/note1.cpp"));
   TUD.Diagnostics.back().Notes.push_back(
@@ -72,6 +76,7 @@
 "Message: 'message #1'\n"
 "FileOffset:  55\n"
 "FilePath:'path/to/source.cpp'\n"
+"FixDescription:  fix1 description\n"
 "Replacements:\n"
 "  - FilePath:'path/to/source.cpp'\n"
 "Offset:  100\n"
@@ -81,6 +86,7 @@
 "Message: 'message #2'\n"
 "FileOffset:  60\n"
 "FilePath:'path/to/header.h'\n"
+"FixDescription:  fix2 description\n"
 "Replacements:\n"
 "  - FilePath:'path/to/header.h'\n"
 "Offset:  62\n"
@@ -97,6 +103,7 @@
 "  - Message: Note2\n"
 "FilePath:'path/to/note2.cpp'\n"
 "FileOffset:  99\n"
+"FixDescription:  ''\n"
 "Replacements:[]\n"
 "...\n",
 YamlContentStream.str());
@@ -110,6 +117,7 @@
 "Message: 'message #1'\n"
 "FileOffset:  55\n"
 "FilePath:path/to/source.cpp\n"
+"FixDescription:  fix1 description\n"
 "Replacements:\n"
 "  - FilePath:path/to/source.cpp\n"
 "Offset:  100\n"
@@ -160,6 +168,7 @@
   EXPECT_EQ("message #1", D1.Message.Message);
   EXPECT_EQ(55u, D1.Message.FileOffset);
   EXPECT_EQ("path/to/source.cpp", D1.Message.FilePath);
+  EXPECT_EQ("fix1 description", D1.FixDescription);
   std::vector Fixes1 = getFixes(D1.Fix);
   ASSERT_EQ(1u, Fixes1.size());
   

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D59932#1446533 , @JonasToth wrote:

> I think the idea is good and implementation, too. If we iterate all checks 
> anyway (probably?) we could think about adding a severity to the checks, too?
>
> I know that code-checker and code-compass have something like this to signal 
> importance of problems (say bugprone and cert differ from readability for 
> example).


Also Clazy  split checks by severity level.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I think the idea is good and implementation, too. If we iterate all checks 
anyway (probably?) we could think about adding a severity to the checks, too?

I know that code-checker and code-compass have something like this to signal 
importance of problems (say bugprone and cert differ from readability for 
example).


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/ClangTidyCheck.h:107
+  /// fixes for different cases.
+  ///   - clang compiler diagnostics and clang static analyzer diagnostics are
+  /// not supported.

I think Clang and Clang Static Analyzer should be capitalized.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/ClangTidyCheck.h:109
+  /// not supported.
+  virtual llvm::StringRef fixDescription() { return ""; }
+

return {} could be used instead.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:266
+return I->second.first;
+  return "";
+}

return {} could be used instead.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:274
+return I->second.second;
   return "";
 }

return {} could be used instead.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

This is my first attempt, still missing tests, but it should be in a shape to 
get early feedbacks.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59932



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