[PATCH] D110600: [clang-tidy] Fix add_new_check.py to generate correct list.rst autofix column from relative path

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley created this revision.
mattbeardsley added a reviewer: kbobyrev.
Herald added a subscriber: xazax.hun.
mattbeardsley requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Previously, the code in add_new_check.py that looks for fixit keywords in check 
source files when generating list.rst assumed that the script would only be 
called from its own path. That means it doesn't find any source files for the 
checks it's attempting to scan for, and it defaults to writing out nothing in 
the "Offers fixes" column for all checks. Other parts of add_new_check.py work 
from other paths, just not this part.

After this fix, add_new_check.py's "offers fixes" column generation for 
list.rst will be consistent regardless of what path it's called from by using 
the caller path that's deduced elsewhere already from sys.argv[0].


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110600

Files:
  clang-tools-extra/clang-tidy/add_new_check.py


Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -323,13 +323,13 @@
   def has_auto_fix(check_name):
 dirname, _, check_name = check_name.partition("-")
 
-checkerCode = get_actual_filename(dirname,
-  get_camel_name(check_name) + '.cpp')
+checker_code = get_actual_filename(os.path.join(clang_tidy_path, dirname),
+   get_camel_name(check_name) + '.cpp')
 
-if not os.path.isfile(checkerCode):
+if not os.path.isfile(checker_code):
   return ""
 
-with io.open(checkerCode, encoding='utf8') as f:
+with io.open(checker_code, encoding='utf8') as f:
   code = f.read()
   if 'FixItHint' in code or "ReplacementText" in code or "fixit" in code:
 # Some simple heuristics to figure out if a checker has an autofix or 
not.


Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -323,13 +323,13 @@
   def has_auto_fix(check_name):
 dirname, _, check_name = check_name.partition("-")
 
-checkerCode = get_actual_filename(dirname,
-  get_camel_name(check_name) + '.cpp')
+checker_code = get_actual_filename(os.path.join(clang_tidy_path, dirname),
+   get_camel_name(check_name) + '.cpp')
 
-if not os.path.isfile(checkerCode):
+if not os.path.isfile(checker_code):
   return ""
 
-with io.open(checkerCode, encoding='utf8') as f:
+with io.open(checker_code, encoding='utf8') as f:
   code = f.read()
   if 'FixItHint' in code or "ReplacementText" in code or "fixit" in code:
 # Some simple heuristics to figure out if a checker has an autofix or not.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110600: [clang-tidy] Fix add_new_check.py to generate correct list.rst autofix column from relative path

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

Thanks! Could you help me commit again when you have time? (same info as here 
)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110600

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

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley created this revision.
mattbeardsley added a reviewer: kbobyrev.
Herald added a subscriber: xazax.hun.
mattbeardsley requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
Herald added a project: clang-tools-extra.

The clang-tidy/infrastructure/pr37091.cpp test inherits the top-level 
.clang-tidy configuration because it doesn't specify its own checks, which ends 
up including misc-RenameFcn. This results in an assert when built with 
"-DCMAKE_BUILD_TYPE=Debug" because fQualifiedName is an empty string in 
"hasName(fQualifiedName)" in RenamefcnCheck.cpp (because none of the config 
options for misc-RenameFcn are provided - not that they should be). The 
HasNameMatcher::HasNameMatcher implementation behind hasName(...) asserts on 
empty strings.

Fixed in 2 ways:

- make the clang-tidy/infrastructure/pr37091.cpp test independent of the 
top-level .clang-tidy (picked an arbitrary check that I saw another 
clang-tidy/infrastructure test was also using: 
clang-tidy/infrastructure/temporaries.cpp)
- It's not useful for the top-level .clang-tidy to have misc-RenameFcn enabled 
anyway, so disabled that

(each of those changes fixes the debug-only assert in 
clang-tidy/infrastructure/pr37091.cpp on its own, but I thought each change had 
its own merits)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114034

Files:
  .clang-tidy
  clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
@@ -6,5 +6,5 @@
 //
 // Verify that no temporary files are left behind by the clang-tidy invocation.
 
-// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy 
-checks='-*,clang-analyzer-core.NullDereference' %s -- --target=mips64
 // RUN: rmdir %t
Index: .clang-tidy
===
--- .clang-tidy
+++ .clang-tidy
@@ -1,4 +1,4 @@
-Checks: 
'-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,readability-identifier-naming'
+Checks: 
'-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,-misc-RenameFcn,readability-identifier-naming'
 CheckOptions:
   - key: readability-identifier-naming.ClassCase
 value:   CamelCase


Index: clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
@@ -6,5 +6,5 @@
 //
 // Verify that no temporary files are left behind by the clang-tidy invocation.
 
-// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy -checks='-*,clang-analyzer-core.NullDereference' %s -- --target=mips64
 // RUN: rmdir %t
Index: .clang-tidy
===
--- .clang-tidy
+++ .clang-tidy
@@ -1,4 +1,4 @@
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,readability-identifier-naming'
+Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,-misc-RenameFcn,readability-identifier-naming'
 CheckOptions:
   - key: readability-identifier-naming.ClassCase
 value:   CamelCase
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a subscriber: dstenb.
mattbeardsley added a comment.

@dstenb - wanted to check with you too to make sure that this change to 
pr37091.cpp seems like it won't interfere with the original intent of the test 
(https://reviews.llvm.org/D45686 )

The lingering file issue you'd fixed seemed to be independent of any particular 
clang-tidy check, but if not, hopefully it can at least get away with not 
relying on the top-level .clang-tidy anymore!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114034

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

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley updated this revision to Diff 388329.
mattbeardsley edited the summary of this revision.
mattbeardsley added a comment.

Well, I've goofed. I was working in a shared fork where someone added a 
RenameFcn check locally (not sure we should be doing that, but that's besides 
the point). I thought I was in my vanilla llvm-project fork when I saw the 
failure. So some of the earlier messages don't apply.

I still think it'd be good to isolate pr37091.cpp from the top-level 
.clang-tidy! But I've reverted the top-level .clang-tidy change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114034

Files:
  clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
@@ -6,5 +6,5 @@
 //
 // Verify that no temporary files are left behind by the clang-tidy invocation.
 
-// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy 
-checks='-*,clang-analyzer-core.NullDereference' %s -- --target=mips64
 // RUN: rmdir %t


Index: clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/pr37091.cpp
@@ -6,5 +6,5 @@
 //
 // Verify that no temporary files are left behind by the clang-tidy invocation.
 
-// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy %s -- --target=mips64
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t clang-tidy -checks='-*,clang-analyzer-core.NullDereference' %s -- --target=mips64
 // RUN: rmdir %t
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114034: [clang-tidy] fix debug-only test failure

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

@dstenb Thanks for the confirmation!

@kbobyrev Sorry about my mixup! Would you be able to help me commit this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114034

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

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.
Herald added a subscriber: carlosgalvezp.
Herald added a project: All.

Someone else ultimately fixed this (slightly differently) last month:
https://reviews.llvm.org/D146875


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210

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

[PATCH] D116137: [clang-tidy] Remove dead plugin code

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley created this revision.
mattbeardsley added reviewers: thakis, hans.
Herald added subscribers: carlosgalvezp, xazax.hun, mgorny.
mattbeardsley requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang-tools-extra.

The following removed the only use of the clang-tidy plugin code:
https://reviews.llvm.org/D97693

This removes the rest of the now-unused and now-untested clang-tidy plugin code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116137

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
  clang-tools-extra/clang-tidy/plugin/ClangTidyPlugin.cpp

Index: clang-tools-extra/clang-tidy/plugin/ClangTidyPlugin.cpp
===
--- clang-tools-extra/clang-tidy/plugin/ClangTidyPlugin.cpp
+++ /dev/null
@@ -1,86 +0,0 @@
-//===- ClangTidyPlugin.cpp - clang-tidy as a clang plugin -===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#include "../ClangTidy.h"
-#include "../ClangTidyDiagnosticConsumer.h"
-#include "../ClangTidyForceLinker.h"
-#include "../ClangTidyModule.h"
-#include "clang/Frontend/CompilerInstance.h"
-#include "clang/Frontend/FrontendPluginRegistry.h"
-#include "clang/Frontend/MultiplexConsumer.h"
-
-namespace clang {
-namespace tidy {
-
-/// The core clang tidy plugin action. This just provides the AST consumer and
-/// command line flag parsing for using clang-tidy as a clang plugin.
-class ClangTidyPluginAction : public PluginASTAction {
-  /// Wrapper to grant the context and diagnostics engine the same lifetime as
-  /// the action.
-  /// We use MultiplexConsumer to avoid writing out all the forwarding methods.
-  class WrapConsumer : public MultiplexConsumer {
-std::unique_ptr Context;
-std::unique_ptr DiagEngine;
-
-  public:
-WrapConsumer(std::unique_ptr Context,
- std::unique_ptr DiagEngine,
- std::vector> Consumer)
-: MultiplexConsumer(std::move(Consumer)), Context(std::move(Context)),
-  DiagEngine(std::move(DiagEngine)) {}
-  };
-
-public:
-  std::unique_ptr CreateASTConsumer(CompilerInstance &Compiler,
- StringRef File) override {
-// Create and set diagnostics engine
-auto *DiagConsumer =
-new ClangTidyDiagnosticConsumer(*Context, &Compiler.getDiagnostics());
-auto DiagEngine = std::make_unique(
-new DiagnosticIDs, new DiagnosticOptions, DiagConsumer);
-Context->setDiagnosticsEngine(DiagEngine.get());
-
-// Create the AST consumer.
-ClangTidyASTConsumerFactory Factory(*Context);
-std::vector> Vec;
-Vec.push_back(Factory.createASTConsumer(Compiler, File));
-
-return std::make_unique(
-std::move(Context), std::move(DiagEngine), std::move(Vec));
-  }
-
-  bool ParseArgs(const CompilerInstance &,
- const std::vector &Args) override {
-ClangTidyGlobalOptions GlobalOptions;
-ClangTidyOptions DefaultOptions;
-ClangTidyOptions OverrideOptions;
-
-// Parse the extra command line args.
-// FIXME: This is very limited at the moment.
-for (StringRef Arg : Args)
-  if (Arg.startswith("-checks="))
-OverrideOptions.Checks = std::string(Arg.substr(strlen("-checks=")));
-
-auto Options = std::make_unique(
-GlobalOptions, DefaultOptions, OverrideOptions);
-Context = std::make_unique(std::move(Options));
-return true;
-  }
-
-private:
-  std::unique_ptr Context;
-};
-} // namespace tidy
-} // namespace clang
-
-// This anchor is used to force the linker to link in the generated object file
-// and thus register the clang-tidy plugin.
-volatile int ClangTidyPluginAnchorSource = 0;
-
-static clang::FrontendPluginRegistry::Add
-X("clang-tidy", "clang-tidy");
Index: clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/plugin/CMakeLists.txt
+++ /dev/null
@@ -1,20 +0,0 @@
-add_clang_library(clangTidyPlugin
-  ClangTidyPlugin.cpp
-
-  LINK_LIBS
-  clangTidy
-  ${ALL_CLANG_TIDY_CHECKS}
-
-  DEPENDS
-  omp_gen
-  )
-
-clang_target_link_libraries(clangTidyPlugin
-  PRIVATE
-  clangAST
-  clangASTMatchers
-  clangBasic
-  clangFrontend
-  clangSema
-  clangTooling
-  )
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools

[PATCH] D110600: [clang-tidy] Fix add_new_check.py to generate correct list.rst autofix column from relative path

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

Works for me! 🙂 I didn't realize you can pull the info automatically from arc. 
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110600

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

[PATCH] D109127: Use python 3 in add_new_check.py and rename_check.py

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley created this revision.
mattbeardsley added a reviewer: kbobyrev.
mattbeardsley requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

As of this commit:

  https://github.com/llvm/llvm-project/commit/307b1fdd

If either of those scripts are invoked with python 2, neither works due to:

  "TypeError: write() argument 1 must be unicode, not str"

Figured I'd send a quick review for the easier fix of "just use python 3", given
that these scripts already don't work with python 2.

Let me know if these are indeed expected/required to continue working with
python 2 though, and I'll look into fixing the "f.write" calls instead.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109127

Files:
  clang-tools-extra/clang-tidy/add_new_check.py
  clang-tools-extra/clang-tidy/rename_check.py


Index: clang-tools-extra/clang-tidy/rename_check.py
===
--- clang-tools-extra/clang-tidy/rename_check.py
+++ clang-tools-extra/clang-tidy/rename_check.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3
 #
 #===- rename_check.py - clang-tidy check renamer *- python -*--===#
 #
@@ -13,6 +13,9 @@
 import io
 import os
 import re
+import sys
+
+assert sys.version_info >= (3,), "Requires python 3"
 
 def replaceInFileRegex(fileName, sFrom, sTo):
   if sFrom == sTo:
Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3
 #
 #===- add_new_check.py - clang-tidy check generator -*- python -*--===#
 #
@@ -16,6 +16,8 @@
 import re
 import sys
 
+assert sys.version_info >= (3,), "Requires python 3"
+
 # Adapts the module's CMakelist file. Returns 'True' if it could add a new
 # entry and 'False' if the entry already existed.
 def adapt_cmake(module_path, check_name_camel):


Index: clang-tools-extra/clang-tidy/rename_check.py
===
--- clang-tools-extra/clang-tidy/rename_check.py
+++ clang-tools-extra/clang-tidy/rename_check.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3
 #
 #===- rename_check.py - clang-tidy check renamer *- python -*--===#
 #
@@ -13,6 +13,9 @@
 import io
 import os
 import re
+import sys
+
+assert sys.version_info >= (3,), "Requires python 3"
 
 def replaceInFileRegex(fileName, sFrom, sTo):
   if sFrom == sTo:
Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3
 #
 #===- add_new_check.py - clang-tidy check generator -*- python -*--===#
 #
@@ -16,6 +16,8 @@
 import re
 import sys
 
+assert sys.version_info >= (3,), "Requires python 3"
+
 # Adapts the module's CMakelist file. Returns 'True' if it could add a new
 # entry and 'False' if the entry already existed.
 def adapt_cmake(module_path, check_name_camel):
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109127: Fix python 2-vs-3 issues in add_new_check.py and rename_check.py

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley updated this revision to Diff 370393.
mattbeardsley retitled this revision from "Use python 3 in add_new_check.py and 
rename_check.py" to "Fix python 2-vs-3 issues in add_new_check.py and 
rename_check.py".
mattbeardsley edited the summary of this revision.
mattbeardsley added a comment.

Reverted python-3-only enforcement, fixed both scripts to work with both python 
2 and python 3
(generally based on the guidance here: 
https://python-future.org/compatible_idioms.html#strings-and-bytes)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109127

Files:
  clang-tools-extra/clang-tidy/add_new_check.py
  clang-tools-extra/clang-tidy/rename_check.py

Index: clang-tools-extra/clang-tidy/rename_check.py
===
--- clang-tools-extra/clang-tidy/rename_check.py
+++ clang-tools-extra/clang-tidy/rename_check.py
@@ -8,6 +8,8 @@
 #
 #===---===#
 
+from __future__ import unicode_literals
+
 import argparse
 import glob
 import io
@@ -117,7 +119,7 @@
   return False
 
   print('Updating %s...' % filename)
-  with io.open(filename, 'wb', encoding='utf8') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
 cpp_found = False
 file_added = False
 for line in lines:
@@ -125,21 +127,23 @@
   if (not file_added) and (cpp_line or cpp_found):
 cpp_found = True
 if (line.strip() > cpp_file) or (not cpp_line):
-  f.write(('  ' + cpp_file + '\n').encode())
+  f.write('  ' + cpp_file + '\n')
   file_added = True
-  f.write(line.encode())
+  f.write(line)
 
   return True
 
 # Modifies the module to include the new check.
 def adapt_module(module_path, module, check_name, check_name_camel):
-  modulecpp = next(filter(lambda p: p.lower() == module.lower() + 'tidymodule.cpp', os.listdir(module_path)))
+  modulecpp = next(iter(filter(
+  lambda p: p.lower() == module.lower() + 'tidymodule.cpp',
+  os.listdir(module_path
   filename = os.path.join(module_path, modulecpp)
   with io.open(filename, 'r', encoding='utf8') as f:
 lines = f.readlines()
 
   print('Updating %s...' % filename)
-  with io.open(filename, 'wb', encoding='utf8') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
 header_added = False
 header_found = False
 check_added = False
@@ -153,21 +157,21 @@
   header_found = True
   if match.group(1) > check_name_camel:
 header_added = True
-f.write(('#include "' + check_name_camel + '.h"\n').encode())
+f.write('#include "' + check_name_camel + '.h"\n')
 elif header_found:
   header_added = True
-  f.write(('#include "' + check_name_camel + '.h"\n').encode())
+  f.write('#include "' + check_name_camel + '.h"\n')
 
   if not check_added:
 if line.strip() == '}':
   check_added = True
-  f.write(check_decl.encode())
+  f.write(check_decl)
 else:
   match = re.search('registerCheck<(.*)>', line)
   if match and match.group(1) > check_name_camel:
 check_added = True
-f.write(check_decl.encode())
-  f.write(line.encode())
+f.write(check_decl)
+  f.write(line)
 
 
 # Adds a release notes entry.
@@ -182,7 +186,7 @@
   checkMatcher = re.compile('- The \'(.*)')
 
   print('Updating %s...' % filename)
-  with io.open(filename, 'wb', encoding='utf8') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
 note_added = False
 header_found = False
 add_note_here = False
@@ -202,22 +206,22 @@
 
 if match:
   header_found = True
-  f.write(line.encode())
+  f.write(line)
   continue
 
 if line.startswith(''):
-  f.write(line.encode())
+  f.write(line)
   continue
 
 if header_found and add_note_here:
   if not line.startswith(''):
-f.write(("""- The '%s' check was renamed to :doc:`%s
+f.write("""- The '%s' check was renamed to :doc:`%s
   `
 
-""" % (old_check_name, new_check_name, new_check_name)).encode())
+""" % (old_check_name, new_check_name, new_check_name))
 note_added = True
 
-  f.write(line.encode())
+  f.write(line)
 
 def main():
   parser = argparse.ArgumentParser(description='Rename clang-tidy check.')
@@ -263,9 +267,9 @@
 (check_name_camel, cmake_lists))
   return 1
 
-modulecpp = next(filter(
+modulecpp = next(iter(filter(
 lambda p: p.lower() == old_module.lower() + 'tidymodule.cpp',
-os.listdir(old_module_path)))
+os.listdir(old_module_path
 deleteMatchingLines(os.path.join(old_module_path, modulecpp),
   '\\b' + check_name_camel + '|\\b' + args.old_check_name)
 
Index: clang

[PATCH] D109127: Fix python 2-vs-3 issues in add_new_check.py and rename_check.py

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

No worries! Figured I'd start the conversation with the easy fix, but was fully 
prepared to hear that we can't let go of python 2 just yet, as much as we may 
want to :). 
Let me know if this fix looks suitable. Thanks for your time!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109127

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

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley created this revision.
mattbeardsley added reviewers: njames93, aaron.ballman.
Herald added a subscriber: xazax.hun.
mattbeardsley requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Per this commit, "Note" diagnostics should not be used to provide the default
fix behavior:
https://reviews.llvm.org/rGabbe9e227ed31e5dde9bb7567bb9f0dd047689c6

With the default clang-tidy behavior being to not "--fix-notes" in the above
commit, the add_new_check.py script would generate a check that does not pass
its own generated test (before this change)

Because the generated example clang-tidy check has a clear default FixItHint,
based on the above commit, the FixItHint would be better suited by being sent to
the warning diag(...), as opposed to keeping the separate note diag(...) and
passing "--fix-notes" in the generated test


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109210

Files:
  clang-tools-extra/clang-tidy/add_new_check.py


Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -144,8 +144,7 @@
   if (!MatchedDecl->getIdentifier() || 
MatchedDecl->getName().startswith("awesome_"))
 return;
   diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
-  << MatchedDecl;
-  diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
+  << MatchedDecl
   << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
 }
 


Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -144,8 +144,7 @@
   if (!MatchedDecl->getIdentifier() || MatchedDecl->getName().startswith("awesome_"))
 return;
   diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
-  << MatchedDecl;
-  diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
+  << MatchedDecl
   << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

In D109210#2981912 , @whisperity 
wrote:

> Uuuh.. I get the sentiment, but this change breaks the very essence of the 
> joke that the default generated code wants to pass on. It also does not 
> showcase that we can emit notes, not just warnings.
>
> How about `function %0 is insufficiently awesome, mark it as 'awesome_'!` in 
> the warning text? (Note how the fix and the note's text diverged already.)
>
> And so we should come up with a joke for the Note tag.
>
> BTW, the test wasn't checking for the note's message at all?

I'm certainly not trying to be a buzzkill on the joke :)
Yes, I did notice that the printed diagnostic text changed from its original, 
but I'll add some background investigation that I should have written in the 
original review text, sorry about laying out quite an incomplete thought 
process. 
At least, I'm hoping I can show there's some extent of due diligence 
behind-the-scenes, and that I'm not just trying to change this willy-nilly!

I read/grepped/etc through several (but certainly not all) existing checks to 
try to identify what the "canonical" diagnostic-and-fix pattern looks like. 
I was thinking that a decent goal would be that, if someone takes the files 
generated by add_new_check.py as their starting point and runs with it, they'll 
end up with check behavior that's reasonably consistent with a "typical" 
existing check.

Here are my observations w.r.t "note" diagnostic usage and diagnostic message 
wording:

1. < ~15% of existing checks use "note" diagnostics

  $ cd clang-tools-extra/clang-tidy
  
  # 40 checks use notes
  $ find . -name '*Check.cpp' | xargs grep -nl 'DiagnosticIDs::Note' | wc -l
  40
  
  # out of 265 calling diag directly
  # (there are 287 total *Check.cpp - most of the remaining 22 are 
android/Cloexec*.cpp which call "CloexecCheck::replaceFunc", which also doesn't 
use DiagnosticIDs::Note)
  $ find . -name '*Check.cpp' | xargs grep -nl 'diag(' | wc -l
  265

2. Out of several messages that I skimmed through arbitrarily (subject to 
potentially being a misrepresentative sample, but hopefully not), the general 
pattern seems to be to state the problem, but not the fix in the handwritten 
diagnostic message text. Here are some examples from the readability module:
  - `readability-redundant-string-init` warns "redundant string 
initialization", but doesn't state "remove the initialization"
  - `readability-redundant-string-cstr` warns "redundant call to %0", but 
doesn't state "remove the call"
  - `readability-named-parameter` warns "all parameters should be named in a 
function", but doesn't state "insert the commented parameter name"
  - `readability-implicit-bool-conversion` warns "implicit conversion bool -> 
%0", but doesn't state the various insertions and removals it's going to do
  - `readability-braces-around-statements` warns "statement should be inside 
braces", but doesn't state "insert statements"

The typical pattern appeared to be to state the issue, then let the FixItHint 
display the suggested conversion, without writing text describing that 
conversion explicitly.
So I aimed to adjust the add_new_check.py script so that it would produce an 
example that's consistent with those observed best practices from the live 
code, so that a new check based on add_new_check.py would look reasonably 
similar to surrounding checks. 
My interpretation of that was to state the issue ("insufficiently awesome"), 
but not describe the insertion - leaving that to the FixItHint::CreateInsertion 
to emit as it sees fit.

Then, since a relatively small percentage of checks use Note diagnostics, my 
take on it was that including the Note example would lead a newcomer to 
unnecessarily start adding Note diagnostics. 
This is anecdotal evidence obviously, but after a couple years spent (on & off) 
writing about 40 clang-tidy checks in my organization, I only just learned 
yesterday from a git bisect that I'd been incorrectly using note diagnostics! 
("why did all my tests start failing after pulling from upstream!"). 
And I'd been using Note diagnostics that way all along because my impression 
from the add_new_check.py example was that streaming the FixItHints into Note 
diagnostics was the right thing to do for the "standard" fix in a clang-tidy 
check.

Last - given relatively few (~15%) checks actually use Note diagnostics at all, 
it didn't seem like Note diagnostics had "earned" their place in the generated 
example code, and could be left as a slightly more "advanced maneuver" instead
Slightly more (42, still about 15%) Check.cpp files (using similar find/grep as 
above methodology) use "Lexer::getSourceText," but that's not part of the 
add_new_check.py example. 
So I guess I'm left wondering - what drives Note diagnostics to be a crucial 
piece to keep as a demo in add_new_check.py, as opposed to showcasing a 
different maneuver? Is there something else that would be more

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

Ah, and to your direct question:

> BTW, the test wasn't checking for the note's message at all?"

Right, the generated test file does not ever do `// CHECK-MESSAGES: :Row:Col: 
note: insert 'awesome'` (nor does the LIT setup here enforce that notes need to 
be checked - it enforces that all warning and error diagnostics must be 
checked, but it looks like it leaves out note diagnostics)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210

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

[PATCH] D109127: Fix python 2-vs-3 issues in add_new_check.py and rename_check.py

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

Thanks! I don't have commit access, I was just reading here 
 what to do:

> Prior to obtaining commit access, it is common practice to request that 
> someone with commit access commits on your behalf. When doing so, please 
> provide the name and email address you would like to use in the Author 
> property of the commit.

When you have a chance, would you be able to help with that? Here's my info:
Matt Beardsley
mbear...@mathworks.com


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109127

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

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

Thanks for your reply and sorry about my very sluggish reply!
I am looking into updating the docs as you suggested, and that got me looking 
at this doc page 
.
 Interestingly, that doc page's version of the add_new_check.py template 
doesn't have the `Note` diagnostic. So that got me digging into the history.
It seems like 2 commits are at odds with each other about what the right thing 
to do is with `FixItHint`s w.r.t. `Note` vs `Warning` diagnostics.

1. This commit 

 says that "fix descriptions and fixes are emitted via diagnostic::Note (rather 
than attaching the main warning diagnostic)."  - And note how that commit 
changes add_new_check.py - adding the `Note` diagnostic!
2. This commit 
 seems to 
suggest that Note diagnostics should //not// be used to provide the "standard" 
fix (and generally enforces that by `--fix-notes` being off by default)

It looks like nearly all existing check implementations disagree with the 
former, and agree with the latter.
Those two commits appear to be at odds with each other on the semantics of 
`Note` diagnostics - any opinions on how to reconcile that apparent 
disagreement?

My (limited) perspective is:
Since `--fix-notes` is off by default, it seems like `FixIt`s on `Note` 
diagnostics are pretty well enforced to be used for secondary/uncertain 
suggestions, while `FixIt`s on `Warning` diagnostics are for the primary 
fix(es). 
That seems to rule out accepting the guidance of first commit w.r.t. attaching 
fixes to `Note` diagnostics given the `--fix-notes` flag imposes that such 
note-fixes are implicitly secondary to warning-fixes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210

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