[PATCH] D56343: [clang-tidy] Refactor: Compose Method on check_clang_tidy.py

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 190789.
LegalizeAdulthood added a comment.
Herald added a subscriber: jdoerfert.

To avoid passing lots of state to/from extracted functions, extract a class 
instead and use member variables for most of the state


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

https://reviews.llvm.org/D56343

Files:
  test/clang-tidy/check_clang_tidy.py

Index: test/clang-tidy/check_clang_tidy.py
===
--- test/clang-tidy/check_clang_tidy.py
+++ test/clang-tidy/check_clang_tidy.py
@@ -38,170 +38,175 @@
 f.write(text)
 f.truncate()
 
+
 def csv(string):
   return string.split(',')
 
-def main():
-  parser = argparse.ArgumentParser()
-  parser.add_argument('-expect-clang-tidy-error', action='store_true')
-  parser.add_argument('-resource-dir')
-  parser.add_argument('-assume-filename')
-  parser.add_argument('input_file_name')
-  parser.add_argument('check_name')
-  parser.add_argument('temp_file_name')
-  parser.add_argument('-check-suffix', '-check-suffixes',
-  default=[''], type=csv,
-  help="comma-separated list of FileCheck suffixes")
-
-  args, extra_args = parser.parse_known_args()
-
-  resource_dir = args.resource_dir
-  assume_file_name = args.assume_filename
-  input_file_name = args.input_file_name
-  check_name = args.check_name
-  temp_file_name = args.temp_file_name
-  expect_clang_tidy_error = args.expect_clang_tidy_error
-
-  file_name_with_extension = assume_file_name or input_file_name
-  _, extension = os.path.splitext(file_name_with_extension)
-  if extension not in ['.c', '.hpp', '.m', '.mm']:
-extension = '.cpp'
-  temp_file_name = temp_file_name + extension
-
-  clang_tidy_extra_args = extra_args
-  if len(clang_tidy_extra_args) == 0:
-clang_tidy_extra_args = ['--']
-if extension in ['.cpp', '.hpp', '.mm']:
-  clang_tidy_extra_args.append('--std=c++11')
-if extension in ['.m', '.mm']:
-  clang_tidy_extra_args.extend(
-  ['-fobjc-abi-version=2', '-fobjc-arc'])
-
-  # Tests should not rely on STL being available, and instead provide mock
-  # implementations of relevant APIs.
-  clang_tidy_extra_args.append('-nostdinc++')
-
-  if resource_dir is not None:
-clang_tidy_extra_args.append('-resource-dir=%s' % resource_dir)
-
-  with open(input_file_name, 'r') as input_file:
-input_text = input_file.read()
-
-  check_fixes_prefixes = []
-  check_messages_prefixes = []
-  check_notes_prefixes = []
-
-  has_check_fixes = False
-  has_check_messages = False
-  has_check_notes = False
-
-  for check in args.check_suffix:
-if check and not re.match('^[A-Z0-9\-]+$', check):
-  sys.exit('Only A..Z, 0..9 and "-" are ' +
-'allowed in check suffixes list, but "%s" was given' % (check))
-
-file_check_suffix = ('-' + check) if check else ''
-check_fixes_prefix = 'CHECK-FIXES' + file_check_suffix
-check_messages_prefix = 'CHECK-MESSAGES' + file_check_suffix
-check_notes_prefix = 'CHECK-NOTES' + file_check_suffix
-
-has_check_fix = check_fixes_prefix in input_text
-has_check_message = check_messages_prefix in input_text
-has_check_note = check_notes_prefix in input_text
-
-if has_check_note and has_check_message:
-  sys.exit('Please use either %s or %s but not both' %
-(check_notes_prefix, check_messages_prefix))
-
-if not has_check_fix and not has_check_message and not has_check_note:
-  sys.exit('%s, %s or %s not found in the input' %
-(check_fixes_prefix, check_messages_prefix, check_notes_prefix))
-
-has_check_fixes = has_check_fixes or has_check_fix
-has_check_messages = has_check_messages or has_check_message
-has_check_notes = has_check_notes or has_check_note
-
-check_fixes_prefixes.append(check_fixes_prefix)
-check_messages_prefixes.append(check_messages_prefix)
-check_notes_prefixes.append(check_notes_prefix)
-
-  assert has_check_fixes or has_check_messages or has_check_notes
-  # Remove the contents of the CHECK lines to avoid CHECKs matching on
-  # themselves.  We need to keep the comments to preserve line numbers while
-  # avoiding empty lines which could potentially trigger formatting-related
-  # checks.
-  cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
-
-  write_file(temp_file_name, cleaned_test)
-
-  original_file_name = temp_file_name + ".orig"
-  write_file(original_file_name, cleaned_test)
-
-  args = ['clang-tidy', temp_file_name, '-fix', '--checks=-*,' + check_name] + \
-clang_tidy_extra_args
-  if expect_clang_tidy_error:
-args.insert(0, 'not')
-  print('Running ' + repr(args) + '...')
-  try:
-clang_tidy_output = \
-subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
-  except subprocess.CalledProcessError as e:
-print('clang-tidy failed:\n' + e.output.decode())
-raise
-
-  print(' clang

[PATCH] D56343: [clang-tidy] Refactor: Extract Class CheckRunner on check_clang_tidy.py

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 9 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: test/clang-tidy/check_clang_tidy.py:112
+process_output = e.output.decode()
+print('%s failed:\n%s' % (' '.join(args), process_output))
+if raise_error:

JonasToth wrote:
> Its better to use `.format()` instead of `%` syntax
That can be done in a later commit; it is not the point of this review.



Comment at: test/clang-tidy/check_clang_tidy.py:141
+   '-check-prefixes=' + ','.join(check_notes_prefixes),
+   '-implicit-check-not={{note|warning|error}}:'])
+  return

serge-sans-paille wrote:
> These three `check_*` function all use the same `'FileCheck', '-']` 
> pattern. Maybe it's worth syndicating that to a 
> try_run_filecheck(input_file0, input_file1, *extra_args)` function.
That can be done in a later commit; it is not the point of this review.



Comment at: test/clang-tidy/check_clang_tidy.py:178
+check_fixes_prefixes, check_messages_prefixes, check_notes_prefixes = \
+get_prefixes(args.check_suffix, input_text)
+

serge-sans-paille wrote:
> This is the verbose call site I was pointing to above.
Addressed by Extract Class instead of Extract Function



Comment at: test/clang-tidy/check_clang_tidy.py:206
 
-  if has_check_fixes:
-try:
-  subprocess.check_output(
-  ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
-   '-check-prefixes=' + ','.join(check_fixes_prefixes),
-   '-strict-whitespace'],
-  stderr=subprocess.STDOUT)
-except subprocess.CalledProcessError as e:
-  print('FileCheck failed:\n' + e.output.decode())
-  raise
-
-  if has_check_messages:
-messages_file = temp_file_name + '.msg'
-write_file(messages_file, clang_tidy_output)
-try:
-  subprocess.check_output(
-  ['FileCheck', '-input-file=' + messages_file, input_file_name,
-   '-check-prefixes=' + ','.join(check_messages_prefixes),
-   '-implicit-check-not={{warning|error}}:'],
-  stderr=subprocess.STDOUT)
-except subprocess.CalledProcessError as e:
-  print('FileCheck failed:\n' + e.output.decode())
-  raise
-
-  if has_check_notes:
-notes_file = temp_file_name + '.notes'
-filtered_output = [line for line in clang_tidy_output.splitlines()
-   if not "note: FIX-IT applied" in line]
-write_file(notes_file, '\n'.join(filtered_output))
-try:
-  subprocess.check_output(
-  ['FileCheck', '-input-file=' + notes_file, input_file_name,
-   '-check-prefixes=' + ','.join(check_notes_prefixes),
-   '-implicit-check-not={{note|warning|error}}:'],
-  stderr=subprocess.STDOUT)
-except subprocess.CalledProcessError as e:
-  print('FileCheck failed:\n' + e.output.decode())
-  raise
+  check_fixes(check_fixes_prefixes, has_check_fixes, input_file_name, 
temp_file_name)
+  check_messages(check_messages_prefixes, has_check_messages, 
clang_tidy_output, input_file_name, temp_file_name)

JonasToth wrote:
> If would prefer keeping the `if check_notes` outside of the function call and 
> remove that one argument. Same for the other `check_...` functions
It's the wrong level of abstraction for the `if` check to be inside `run()`


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

https://reviews.llvm.org/D56343



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

[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.
Herald added a subscriber: jdoerfert.

In D56323#1350998 , @JonasToth wrote:

> @LegalizeAdulthood your stuck on the commit right things right? If you want I 
> can commit for you (maybe even later) as you said you are limited on time.


Yes, please.  I haven't figured out what the new commit procedure is or how I 
can submit.  If you could submit this for me, that would help me out, thanks.


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

https://reviews.llvm.org/D56323



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

[PATCH] D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 4 inline comments as done.
LegalizeAdulthood added inline comments.
Herald added a subscriber: jdoerfert.



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:386
 
-  bool BoolValue = Bool->getValue();
+  const bool BoolValue = Bool->getValue();
 

JonasToth wrote:
> aaron.ballman wrote:
> > LegalizeAdulthood wrote:
> > > JonasToth wrote:
> > > > `const` on values is uncommon in clang-tidy code. Please keep that 
> > > > consistent.
> > > I can change the code, but I don't understand the push back.
> > > 
> > > "That's the way it's done elsewhere" just doesn't seem like good 
> > > reasoning.
> > > 
> > > I write const on values to signify that they are computed once and only 
> > > once.  What is gained by removing that statement of once-and-only-once?
> > > "That's the way it's done elsewhere" just doesn't seem like good 
> > > reasoning.
> > 
> > Keeping the code consistent with the vast majority of the remainder of the 
> > project is valid reasoning. I am echoing the request to drop the top-level 
> > const. We don't use this pattern for non-pointer/reference types and 
> > there's not a whole lot gained by using it inconsistently.
> Plus we do have a clang-tidy check (in the pipeline) that could do that 
> automatically if the LLVM projects decides to go that route. So we won't 
> suffer in the future, if we add the const.
You haven't addressed my point, which is that `const` is for values that don't 
change.  Instead, you're just repeating "we haven't done it that way" instead 
of refuting the utility of `const`.


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

https://reviews.llvm.org/D56303



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

[PATCH] D18509: clang-tidy: add_new_check.py stubs out release notes

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood abandoned this revision.
LegalizeAdulthood added a comment.
Herald added a reviewer: serge-sans-paille.

Review takes too long to make forward progress; abandoning.


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

https://reviews.llvm.org/D18509



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

[PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood abandoned this revision.
LegalizeAdulthood added a comment.
Herald added subscribers: jdoerfert, mgorny.

Review takes too long to make forward progress; abandoning.


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

https://reviews.llvm.org/D7982



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

[PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood abandoned this revision.
LegalizeAdulthood added a comment.
Herald added a subscriber: xazax.hun.
Herald added a reviewer: serge-sans-paille.

Review process takes too long to make forward progress; abandoning.


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

https://reviews.llvm.org/D17482



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

[PATCH] D56303: Handle case/default statements when simplifying boolean expressions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: cfe-commits.
LegalizeAdulthood added a project: clang-tools-extra.

Structure this similarly to the way replaceCompoundReturnWithCondition works.

- Add readability-simplify-boolean-expr test cases for case stmt
- Add readability-simplify-boolean-expr test cases for default stmt
- Add negative test cases

Fixes #26704


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56303

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tidy/readability/SimplifyBooleanExprCheck.h
  test/clang-tidy/readability-simplify-bool-expr-case.cpp

Index: test/clang-tidy/readability-simplify-bool-expr-case.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-case.cpp
@@ -0,0 +1,586 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+bool switch_stmt(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+if (b == true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 1:
+if (b == false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(!b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 2:
+if (b && true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 3:
+if (b && false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(false\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 4:
+if (b || true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(true\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+// CHECK-FIXES-NEXT: {{break;}}
+
+  case 5:
+if (b || false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 6:
+return i > 0 ? true : false;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: {{return i > 0;}}
+
+  case 7:
+return i > 0 ? false : true;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: {{return i <= 0;}}
+
+  case 8:
+if (true)
+  j = 10;
+else
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+// CHECK-FIXES:  {{j = 10;$}}
+// CHECK-FIXES-NEXT: {{break;$}}
+
+  case 9:
+if (false)
+  j = -20;
+else
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+// CHECK-FIXES: {{j = 10;}}
+// CHECK-FIXES-NEXT: {{break;}}
+
+  case 10:
+if (j > 10)
+  return true;
+else
+  return false;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{return j > 10;}}
+
+  case 11:
+if (j > 10)
+  return false;
+else
+  return true;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{return j <= 10;}}
+
+  case 12:
+if (j > 10)
+  b = true;
+else
+  b = false;
+return b;
+// CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{b = j > 10;}}
+// CHECK-FIXES-NEXT: {{return b;}}
+
+  case 13:
+if (j > 10)
+  b = false;
+else
+  b = true;
+return b;
+// CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{b = j <= 10;}}
+// CHECK-FIXES-NEXT: {{return b;}}
+
+  case 14:
+if (j > 10)
+  return true;
+return false;
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+// FIXES: {{return j > 10;}}
+
+  case 15:
+if (j > 10)
+  return false;
+return true;
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+// FIXES: {{return j <= 10;}}
+
+  case 16:
+if (j > 10)
+  return true;
+return true;
+return false;
+
+  case 17:
+if (j > 10)
+  return false;
+return false;
+return true;
+
+  case 100: {
+if (b == true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+  }
+
+  case 101: {
+if (b == false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(!b\)}}
+// CHECK-FI

[PATCH] D56303: Handle case/default statements when simplifying boolean expressions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 180184.
LegalizeAdulthood added a comment.

clang-format


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

https://reviews.llvm.org/D56303

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tidy/readability/SimplifyBooleanExprCheck.h
  test/clang-tidy/readability-simplify-bool-expr-case.cpp

Index: test/clang-tidy/readability-simplify-bool-expr-case.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-case.cpp
@@ -0,0 +1,586 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+bool switch_stmt(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+if (b == true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 1:
+if (b == false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(!b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 2:
+if (b && true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 3:
+if (b && false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(false\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 4:
+if (b || true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(true\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+// CHECK-FIXES-NEXT: {{break;}}
+
+  case 5:
+if (b || false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 6:
+return i > 0 ? true : false;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: {{return i > 0;}}
+
+  case 7:
+return i > 0 ? false : true;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: {{return i <= 0;}}
+
+  case 8:
+if (true)
+  j = 10;
+else
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+// CHECK-FIXES:  {{j = 10;$}}
+// CHECK-FIXES-NEXT: {{break;$}}
+
+  case 9:
+if (false)
+  j = -20;
+else
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+// CHECK-FIXES: {{j = 10;}}
+// CHECK-FIXES-NEXT: {{break;}}
+
+  case 10:
+if (j > 10)
+  return true;
+else
+  return false;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{return j > 10;}}
+
+  case 11:
+if (j > 10)
+  return false;
+else
+  return true;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{return j <= 10;}}
+
+  case 12:
+if (j > 10)
+  b = true;
+else
+  b = false;
+return b;
+// CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{b = j > 10;}}
+// CHECK-FIXES-NEXT: {{return b;}}
+
+  case 13:
+if (j > 10)
+  b = false;
+else
+  b = true;
+return b;
+// CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{b = j <= 10;}}
+// CHECK-FIXES-NEXT: {{return b;}}
+
+  case 14:
+if (j > 10)
+  return true;
+return false;
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+// FIXES: {{return j > 10;}}
+
+  case 15:
+if (j > 10)
+  return false;
+return true;
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+// FIXES: {{return j <= 10;}}
+
+  case 16:
+if (j > 10)
+  return true;
+return true;
+return false;
+
+  case 17:
+if (j > 10)
+  return false;
+return false;
+return true;
+
+  case 100: {
+if (b == true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+  }
+
+  case 101: {
+if (b == false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(!b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+  }
+
+  case 102: {
+if (b && true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+  }
+
+  case 103: {
+

[PATCH] D56323: Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 180274.
LegalizeAdulthood added a comment.

Fix typo in id


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

https://reviews.llvm.org/D56323

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  test/clang-tidy/readability-simplify-bool-expr-members.cpp

Index: test/clang-tidy/readability-simplify-bool-expr-members.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-members.cpp
@@ -0,0 +1,360 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+struct X {
+  explicit operator bool();
+};
+
+class A {
+public:
+  int m;
+};
+
+struct S {
+  S() : m_ar(s_a) {}
+
+  void operator_equals();
+  void operator_or();
+  void operator_and();
+  void ternary_operator();
+  void operator_not_equal();
+  void simple_conditional_assignment_statements();
+  void complex_conditional_assignment_statements();
+  void chained_conditional_assignment();
+  bool non_null_pointer_condition();
+  bool null_pointer_condition();
+  bool negated_non_null_pointer_condition();
+  bool negated_null_pointer_condition();
+  bool integer_not_zero();
+  bool member_pointer_nullptr();
+  bool integer_member_implicit_cast();
+  bool expr_with_cleanups();
+
+  bool m_b1 = false;
+  bool m_b2 = false;
+  bool m_b3 = false;
+  bool m_b4 = false;
+  int *m_p = nullptr;
+  int A::*m_m = nullptr;
+  int m_i = 0;
+  A *m_a = nullptr;
+  static A s_a;
+  A &m_ar;
+};
+
+void S::operator_equals() {
+  int i = 0;
+  m_b1 = (i > 2);
+  if (m_b1 == true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 5;
+  } else {
+i = 6;
+  }
+  m_b2 = (i > 4);
+  if (m_b2 == false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(!m_b2\) {$}}
+i = 7;
+  } else {
+i = 9;
+  }
+  m_b3 = (i > 6);
+  if (true == m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b3\) {$}}
+i = 10;
+  } else {
+i = 11;
+  }
+  m_b4 = (i > 8);
+  if (false == m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(!m_b4\) {$}}
+i = 12;
+  } else {
+i = 13;
+  }
+}
+
+void S::operator_or() {
+  int i = 0;
+  m_b1 = (i > 10);
+  if (m_b1 || false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 14;
+  } else {
+i = 15;
+  }
+  m_b2 = (i > 10);
+  if (m_b2 || true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(true\) {$}}
+i = 16;
+  } else {
+i = 17;
+  }
+  m_b3 = (i > 10);
+  if (false || m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b3\) {$}}
+i = 18;
+  } else {
+i = 19;
+  }
+  m_b4 = (i > 10);
+  if (true || m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(true\) {$}}
+i = 20;
+  } else {
+i = 21;
+  }
+}
+
+void S::operator_and() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (m_b1 && false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(false\) {$}}
+i = 22;
+  } else {
+i = 23;
+  }
+  m_b2 = (i > 20);
+  if (m_b2 && true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b2\) {$}}
+i = 24;
+  } else {
+i = 25;
+  }
+  m_b3 = (i > 20);
+  if (false && m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(false\) {$}}
+i = 26;
+  } else {
+i = 27;
+  }
+  m_b4 = (i > 20);
+  if (true && m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b4\) {$}}
+i = 28;
+  } else {
+i = 29;
+  }
+}
+
+void S::ternary_operator() {
+  int i = 0;
+  m_b1 = (i > 20) ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b1 = i > 20;$}}
+
+  m_b2 = (i > 20) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b2 = i <= 20;$}}
+
+  m_b3 = ((i > 20)) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b3 = i <= 20;$}}
+}
+
+void S::operator_not_equal() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (false != m_b1) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 30;
+  } else {
+i = 31;
+  }
+  m_b2 = (i > 20);
+  if (true != m_b2) {
+// CHECK-MESSAGES: :[[@L

[PATCH] D56323: Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: cfe-commits.
LegalizeAdulthood updated this revision to Diff 180274.
LegalizeAdulthood added a comment.

Fix typo in id


- Add readability-simplify-boolean-expr test cases for member variables

Fixes #40179


https://reviews.llvm.org/D56323

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  test/clang-tidy/readability-simplify-bool-expr-members.cpp

Index: test/clang-tidy/readability-simplify-bool-expr-members.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-members.cpp
@@ -0,0 +1,360 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+struct X {
+  explicit operator bool();
+};
+
+class A {
+public:
+  int m;
+};
+
+struct S {
+  S() : m_ar(s_a) {}
+
+  void operator_equals();
+  void operator_or();
+  void operator_and();
+  void ternary_operator();
+  void operator_not_equal();
+  void simple_conditional_assignment_statements();
+  void complex_conditional_assignment_statements();
+  void chained_conditional_assignment();
+  bool non_null_pointer_condition();
+  bool null_pointer_condition();
+  bool negated_non_null_pointer_condition();
+  bool negated_null_pointer_condition();
+  bool integer_not_zero();
+  bool member_pointer_nullptr();
+  bool integer_member_implicit_cast();
+  bool expr_with_cleanups();
+
+  bool m_b1 = false;
+  bool m_b2 = false;
+  bool m_b3 = false;
+  bool m_b4 = false;
+  int *m_p = nullptr;
+  int A::*m_m = nullptr;
+  int m_i = 0;
+  A *m_a = nullptr;
+  static A s_a;
+  A &m_ar;
+};
+
+void S::operator_equals() {
+  int i = 0;
+  m_b1 = (i > 2);
+  if (m_b1 == true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 5;
+  } else {
+i = 6;
+  }
+  m_b2 = (i > 4);
+  if (m_b2 == false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(!m_b2\) {$}}
+i = 7;
+  } else {
+i = 9;
+  }
+  m_b3 = (i > 6);
+  if (true == m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b3\) {$}}
+i = 10;
+  } else {
+i = 11;
+  }
+  m_b4 = (i > 8);
+  if (false == m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(!m_b4\) {$}}
+i = 12;
+  } else {
+i = 13;
+  }
+}
+
+void S::operator_or() {
+  int i = 0;
+  m_b1 = (i > 10);
+  if (m_b1 || false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 14;
+  } else {
+i = 15;
+  }
+  m_b2 = (i > 10);
+  if (m_b2 || true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(true\) {$}}
+i = 16;
+  } else {
+i = 17;
+  }
+  m_b3 = (i > 10);
+  if (false || m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b3\) {$}}
+i = 18;
+  } else {
+i = 19;
+  }
+  m_b4 = (i > 10);
+  if (true || m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(true\) {$}}
+i = 20;
+  } else {
+i = 21;
+  }
+}
+
+void S::operator_and() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (m_b1 && false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(false\) {$}}
+i = 22;
+  } else {
+i = 23;
+  }
+  m_b2 = (i > 20);
+  if (m_b2 && true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b2\) {$}}
+i = 24;
+  } else {
+i = 25;
+  }
+  m_b3 = (i > 20);
+  if (false && m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(false\) {$}}
+i = 26;
+  } else {
+i = 27;
+  }
+  m_b4 = (i > 20);
+  if (true && m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b4\) {$}}
+i = 28;
+  } else {
+i = 29;
+  }
+}
+
+void S::ternary_operator() {
+  int i = 0;
+  m_b1 = (i > 20) ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b1 = i > 20;$}}
+
+  m_b2 = (i > 20) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b2 = i <= 20;$}}
+
+  m_b3 = ((i > 20)) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b3 = i <= 20;$}}
+}
+
+void S::operator_not_equal() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (false != m_b1) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+  

[PATCH] D56323: Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 180275.
LegalizeAdulthood added a comment.

Fix typo in id


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

https://reviews.llvm.org/D56323

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  test/clang-tidy/readability-simplify-bool-expr-members.cpp

Index: test/clang-tidy/readability-simplify-bool-expr-members.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-members.cpp
@@ -0,0 +1,360 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+struct X {
+  explicit operator bool();
+};
+
+class A {
+public:
+  int m;
+};
+
+struct S {
+  S() : m_ar(s_a) {}
+
+  void operator_equals();
+  void operator_or();
+  void operator_and();
+  void ternary_operator();
+  void operator_not_equal();
+  void simple_conditional_assignment_statements();
+  void complex_conditional_assignment_statements();
+  void chained_conditional_assignment();
+  bool non_null_pointer_condition();
+  bool null_pointer_condition();
+  bool negated_non_null_pointer_condition();
+  bool negated_null_pointer_condition();
+  bool integer_not_zero();
+  bool member_pointer_nullptr();
+  bool integer_member_implicit_cast();
+  bool expr_with_cleanups();
+
+  bool m_b1 = false;
+  bool m_b2 = false;
+  bool m_b3 = false;
+  bool m_b4 = false;
+  int *m_p = nullptr;
+  int A::*m_m = nullptr;
+  int m_i = 0;
+  A *m_a = nullptr;
+  static A s_a;
+  A &m_ar;
+};
+
+void S::operator_equals() {
+  int i = 0;
+  m_b1 = (i > 2);
+  if (m_b1 == true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 5;
+  } else {
+i = 6;
+  }
+  m_b2 = (i > 4);
+  if (m_b2 == false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(!m_b2\) {$}}
+i = 7;
+  } else {
+i = 9;
+  }
+  m_b3 = (i > 6);
+  if (true == m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b3\) {$}}
+i = 10;
+  } else {
+i = 11;
+  }
+  m_b4 = (i > 8);
+  if (false == m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(!m_b4\) {$}}
+i = 12;
+  } else {
+i = 13;
+  }
+}
+
+void S::operator_or() {
+  int i = 0;
+  m_b1 = (i > 10);
+  if (m_b1 || false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 14;
+  } else {
+i = 15;
+  }
+  m_b2 = (i > 10);
+  if (m_b2 || true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(true\) {$}}
+i = 16;
+  } else {
+i = 17;
+  }
+  m_b3 = (i > 10);
+  if (false || m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b3\) {$}}
+i = 18;
+  } else {
+i = 19;
+  }
+  m_b4 = (i > 10);
+  if (true || m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(true\) {$}}
+i = 20;
+  } else {
+i = 21;
+  }
+}
+
+void S::operator_and() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (m_b1 && false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(false\) {$}}
+i = 22;
+  } else {
+i = 23;
+  }
+  m_b2 = (i > 20);
+  if (m_b2 && true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b2\) {$}}
+i = 24;
+  } else {
+i = 25;
+  }
+  m_b3 = (i > 20);
+  if (false && m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(false\) {$}}
+i = 26;
+  } else {
+i = 27;
+  }
+  m_b4 = (i > 20);
+  if (true && m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b4\) {$}}
+i = 28;
+  } else {
+i = 29;
+  }
+}
+
+void S::ternary_operator() {
+  int i = 0;
+  m_b1 = (i > 20) ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b1 = i > 20;$}}
+
+  m_b2 = (i > 20) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b2 = i <= 20;$}}
+
+  m_b3 = ((i > 20)) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b3 = i <= 20;$}}
+}
+
+void S::operator_not_equal() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (false != m_b1) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 30;
+  } else {
+i = 31;
+  }
+  m_b2 = (i > 20);
+  if (true != m_b2) {
+// CHECK-MESSAGES: :[[@L

[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 180276.
LegalizeAdulthood added a comment.

No, really, fix typo in id


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56323

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  test/clang-tidy/readability-simplify-bool-expr-members.cpp

Index: test/clang-tidy/readability-simplify-bool-expr-members.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-members.cpp
@@ -0,0 +1,360 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+struct X {
+  explicit operator bool();
+};
+
+class A {
+public:
+  int m;
+};
+
+struct S {
+  S() : m_ar(s_a) {}
+
+  void operator_equals();
+  void operator_or();
+  void operator_and();
+  void ternary_operator();
+  void operator_not_equal();
+  void simple_conditional_assignment_statements();
+  void complex_conditional_assignment_statements();
+  void chained_conditional_assignment();
+  bool non_null_pointer_condition();
+  bool null_pointer_condition();
+  bool negated_non_null_pointer_condition();
+  bool negated_null_pointer_condition();
+  bool integer_not_zero();
+  bool member_pointer_nullptr();
+  bool integer_member_implicit_cast();
+  bool expr_with_cleanups();
+
+  bool m_b1 = false;
+  bool m_b2 = false;
+  bool m_b3 = false;
+  bool m_b4 = false;
+  int *m_p = nullptr;
+  int A::*m_m = nullptr;
+  int m_i = 0;
+  A *m_a = nullptr;
+  static A s_a;
+  A &m_ar;
+};
+
+void S::operator_equals() {
+  int i = 0;
+  m_b1 = (i > 2);
+  if (m_b1 == true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 5;
+  } else {
+i = 6;
+  }
+  m_b2 = (i > 4);
+  if (m_b2 == false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(!m_b2\) {$}}
+i = 7;
+  } else {
+i = 9;
+  }
+  m_b3 = (i > 6);
+  if (true == m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b3\) {$}}
+i = 10;
+  } else {
+i = 11;
+  }
+  m_b4 = (i > 8);
+  if (false == m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(!m_b4\) {$}}
+i = 12;
+  } else {
+i = 13;
+  }
+}
+
+void S::operator_or() {
+  int i = 0;
+  m_b1 = (i > 10);
+  if (m_b1 || false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 14;
+  } else {
+i = 15;
+  }
+  m_b2 = (i > 10);
+  if (m_b2 || true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(true\) {$}}
+i = 16;
+  } else {
+i = 17;
+  }
+  m_b3 = (i > 10);
+  if (false || m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b3\) {$}}
+i = 18;
+  } else {
+i = 19;
+  }
+  m_b4 = (i > 10);
+  if (true || m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(true\) {$}}
+i = 20;
+  } else {
+i = 21;
+  }
+}
+
+void S::operator_and() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (m_b1 && false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(false\) {$}}
+i = 22;
+  } else {
+i = 23;
+  }
+  m_b2 = (i > 20);
+  if (m_b2 && true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b2\) {$}}
+i = 24;
+  } else {
+i = 25;
+  }
+  m_b3 = (i > 20);
+  if (false && m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(false\) {$}}
+i = 26;
+  } else {
+i = 27;
+  }
+  m_b4 = (i > 20);
+  if (true && m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b4\) {$}}
+i = 28;
+  } else {
+i = 29;
+  }
+}
+
+void S::ternary_operator() {
+  int i = 0;
+  m_b1 = (i > 20) ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b1 = i > 20;$}}
+
+  m_b2 = (i > 20) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b2 = i <= 20;$}}
+
+  m_b3 = ((i > 20)) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b3 = i <= 20;$}}
+}
+
+void S::operator_not_equal() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (false != m_b1) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 30;
+  } else {
+i = 31;
+  }
+  m_b2 = (i > 20);
+ 

[PATCH] D56343: [clang-tidy] Refactor: Compose Method on check_clang_tidy.py

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
Herald added a subscriber: xazax.hun.
Herald added a reviewer: serge-sans-paille.

Break up the huge main() function


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56343

Files:
  test/clang-tidy/check_clang_tidy.py

Index: test/clang-tidy/check_clang_tidy.py
===
--- test/clang-tidy/check_clang_tidy.py
+++ test/clang-tidy/check_clang_tidy.py
@@ -42,33 +42,7 @@
 def csv(string):
   return string.split(',')
 
-def main():
-  parser = argparse.ArgumentParser()
-  parser.add_argument('-expect-clang-tidy-error', action='store_true')
-  parser.add_argument('-resource-dir')
-  parser.add_argument('-assume-filename')
-  parser.add_argument('input_file_name')
-  parser.add_argument('check_name')
-  parser.add_argument('temp_file_name')
-  parser.add_argument('-check-suffix', '-check-suffixes',
-  default=[''], type=csv,
-  help="comma-separated list of FileCheck suffixes")
-
-  args, extra_args = parser.parse_known_args()
-
-  resource_dir = args.resource_dir
-  assume_file_name = args.assume_filename
-  input_file_name = args.input_file_name
-  check_name = args.check_name
-  temp_file_name = args.temp_file_name
-  expect_clang_tidy_error = args.expect_clang_tidy_error
-
-  file_name_with_extension = assume_file_name or input_file_name
-  _, extension = os.path.splitext(file_name_with_extension)
-  if extension not in ['.c', '.hpp', '.m', '.mm']:
-extension = '.cpp'
-  temp_file_name = temp_file_name + extension
-
+def get_extra_args(resource_dir, extension, extra_args):
   clang_tidy_extra_args = extra_args
   if len(clang_tidy_extra_args) == 0:
 clang_tidy_extra_args = ['--']
@@ -85,9 +59,9 @@
   if resource_dir is not None:
 clang_tidy_extra_args.append('-resource-dir=%s' % resource_dir)
 
-  with open(input_file_name, 'r') as input_file:
-input_text = input_file.read()
+  return clang_tidy_extra_args
 
+def get_prefixes(check_suffix, input_text):
   check_fixes_prefixes = []
   check_messages_prefixes = []
   check_notes_prefixes = []
@@ -96,7 +70,7 @@
   has_check_messages = False
   has_check_notes = False
 
-  for check in args.check_suffix:
+  for check in check_suffix:
 if check and not re.match('^[A-Z0-9\-]+$', check):
   sys.exit('Only A..Z, 0..9 and "-" are ' +
 'allowed in check suffixes list, but "%s" was given' % (check))
@@ -126,56 +100,23 @@
 check_messages_prefixes.append(check_messages_prefix)
 check_notes_prefixes.append(check_notes_prefix)
 
-  assert has_check_fixes or has_check_messages or has_check_notes
-  # Remove the contents of the CHECK lines to avoid CHECKs matching on
-  # themselves.  We need to keep the comments to preserve line numbers while
-  # avoiding empty lines which could potentially trigger formatting-related
-  # checks.
-  cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
-
-  write_file(temp_file_name, cleaned_test)
-
-  original_file_name = temp_file_name + ".orig"
-  write_file(original_file_name, cleaned_test)
-
-  args = ['clang-tidy', temp_file_name, '-fix', '--checks=-*,' + check_name] + \
-clang_tidy_extra_args
-  if expect_clang_tidy_error:
-args.insert(0, 'not')
-  print('Running ' + repr(args) + '...')
-  try:
-clang_tidy_output = \
-subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
-  except subprocess.CalledProcessError as e:
-print('clang-tidy failed:\n' + e.output.decode())
-raise
-
-  print(' clang-tidy output ---\n' +
-clang_tidy_output +
-'\n--')
-
-  try:
-diff_output = subprocess.check_output(
-['diff', '-u', original_file_name, temp_file_name],
-stderr=subprocess.STDOUT)
-  except subprocess.CalledProcessError as e:
-diff_output = e.output
-
-  print('-- Fixes -\n' +
-diff_output.decode() +
-'\n--')
+  return (has_check_fixes, has_check_messages, has_check_notes, \
+check_fixes_prefixes, check_messages_prefixes, check_notes_prefixes)
 
+def check_fixes(check_fixes_prefixes, has_check_fixes, input_file_name, temp_file_name):
   if has_check_fixes:
 try:
   subprocess.check_output(
   ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
-   '-check-prefixes=' + ','.join(check_fixes_prefixes),
-   '-strict-whitespace'],
+'-check-prefixes=' + ','.join(check_fixes_prefixes),
+'-strict-whitespace'],
   stderr=subprocess.STDOUT)
 except subprocess.CalledProcessError as e:
   print('FileCheck failed:\n' + e.output.decode())
   raise
+  return
 
+def check_messages(check_message

[PATCH] D56343: [clang-tidy] Refactor: Compose Method on check_clang_tidy.py

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 180349.
LegalizeAdulthood added a comment.

Extract Function try_run


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

https://reviews.llvm.org/D56343

Files:
  test/clang-tidy/check_clang_tidy.py

Index: test/clang-tidy/check_clang_tidy.py
===
--- test/clang-tidy/check_clang_tidy.py
+++ test/clang-tidy/check_clang_tidy.py
@@ -42,33 +42,7 @@
 def csv(string):
   return string.split(',')
 
-def main():
-  parser = argparse.ArgumentParser()
-  parser.add_argument('-expect-clang-tidy-error', action='store_true')
-  parser.add_argument('-resource-dir')
-  parser.add_argument('-assume-filename')
-  parser.add_argument('input_file_name')
-  parser.add_argument('check_name')
-  parser.add_argument('temp_file_name')
-  parser.add_argument('-check-suffix', '-check-suffixes',
-  default=[''], type=csv,
-  help="comma-separated list of FileCheck suffixes")
-
-  args, extra_args = parser.parse_known_args()
-
-  resource_dir = args.resource_dir
-  assume_file_name = args.assume_filename
-  input_file_name = args.input_file_name
-  check_name = args.check_name
-  temp_file_name = args.temp_file_name
-  expect_clang_tidy_error = args.expect_clang_tidy_error
-
-  file_name_with_extension = assume_file_name or input_file_name
-  _, extension = os.path.splitext(file_name_with_extension)
-  if extension not in ['.c', '.hpp', '.m', '.mm']:
-extension = '.cpp'
-  temp_file_name = temp_file_name + extension
-
+def get_extra_args(resource_dir, extension, extra_args):
   clang_tidy_extra_args = extra_args
   if len(clang_tidy_extra_args) == 0:
 clang_tidy_extra_args = ['--']
@@ -85,9 +59,9 @@
   if resource_dir is not None:
 clang_tidy_extra_args.append('-resource-dir=%s' % resource_dir)
 
-  with open(input_file_name, 'r') as input_file:
-input_text = input_file.read()
+  return clang_tidy_extra_args
 
+def get_prefixes(check_suffix, input_text):
   check_fixes_prefixes = []
   check_messages_prefixes = []
   check_notes_prefixes = []
@@ -96,7 +70,7 @@
   has_check_messages = False
   has_check_notes = False
 
-  for check in args.check_suffix:
+  for check in check_suffix:
 if check and not re.match('^[A-Z0-9\-]+$', check):
   sys.exit('Only A..Z, 0..9 and "-" are ' +
 'allowed in check suffixes list, but "%s" was given' % (check))
@@ -126,15 +100,91 @@
 check_messages_prefixes.append(check_messages_prefix)
 check_notes_prefixes.append(check_notes_prefix)
 
+  return (has_check_fixes, has_check_messages, has_check_notes, \
+check_fixes_prefixes, check_messages_prefixes, check_notes_prefixes)
+
+def try_run(args, raise_error = True):
+  try:
+process_output = \
+  subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+  except subprocess.CalledProcessError as e:
+process_output = e.output.decode()
+print('%s failed:\n%s' % (' '.join(args), process_output))
+if raise_error:
+  raise
+  return process_output
+
+def check_fixes(check_fixes_prefixes, has_check_fixes, input_file_name, temp_file_name):
+  if has_check_fixes:
+try_run(['FileCheck', '-input-file=' + temp_file_name, input_file_name,
+'-check-prefixes=' + ','.join(check_fixes_prefixes),
+'-strict-whitespace'])
+  return
+
+def check_messages(check_messages_prefixes, has_check_messages, clang_tidy_output, input_file_name, temp_file_name):
+  if has_check_messages:
+messages_file = temp_file_name + '.msg'
+write_file(messages_file, clang_tidy_output)
+try_run(['FileCheck', '-input-file=' + messages_file, input_file_name,
+   '-check-prefixes=' + ','.join(check_messages_prefixes),
+   '-implicit-check-not={{warning|error}}:'])
+  return
+
+def check_notes(check_notes_prefixes, has_check_notes, clang_tidy_output, input_file_name, temp_file_name):
+  if has_check_notes:
+notes_file = temp_file_name + '.notes'
+filtered_output = [line for line in clang_tidy_output.splitlines()
+   if not "note: FIX-IT applied" in line]
+write_file(notes_file, '\n'.join(filtered_output))
+try_run(['FileCheck', '-input-file=' + notes_file, input_file_name,
+   '-check-prefixes=' + ','.join(check_notes_prefixes),
+   '-implicit-check-not={{note|warning|error}}:'])
+  return
+
+def main():
+  parser = argparse.ArgumentParser()
+  parser.add_argument('-expect-clang-tidy-error', action='store_true')
+  parser.add_argument('-resource-dir')
+  parser.add_argument('-assume-filename')
+  parser.add_argument('input_file_name')
+  parser.add_argument('check_name')
+  parser.add_argument('temp_file_name')
+  parser.add_argument('-check-suffix', '-check-suffixes',
+  default=[''], type=csv,
+  help="comma-separated list of FileCheck suffixes")
+
+  args, extra_args = parser.parse_known_args()
+
+  resource_dir = args.r

[PATCH] D56343: [clang-tidy] Refactor: Compose Method on check_clang_tidy.py

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I really want to get these reviewed quickly.  Otherwise, I will run out of 
available time to complete them and get them submitted.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56343



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

[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I really want to get these reviewed quickly.  Otherwise, I will run out of 
available time to complete them and get them submitted.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56323



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

[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 4 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:464
   bool Value, StringRef Id) {
-  auto SimpleThen = binaryOperator(
-  hasOperatorName("="),
-  hasLHS(declRefExpr(hasDeclaration(decl().bind(IfAssignObjId,
-  hasLHS(expr().bind(IfAssignVariableId)),
-  hasRHS(cxxBoolLiteral(equals(Value)).bind(IfAssignLocId)));
-  auto Then = anyOf(SimpleThen, compoundStmt(statementCountIs(1),
- hasAnySubstatement(SimpleThen)));
-  auto SimpleElse = binaryOperator(
-  hasOperatorName("="),
-  hasLHS(declRefExpr(hasDeclaration(equalsBoundNode(IfAssignObjId,
-  hasRHS(cxxBoolLiteral(equals(!Value;
-  auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
- hasAnySubstatement(SimpleElse)));
+  const auto VarAssign =
+  declRefExpr(hasDeclaration(decl().bind(IfAssignVarId)));

JonasToth wrote:
> The `const` for these locals in uncommon in clang-tidy, given its a value 
> type. I am not strongly against them, but would prefer consistency.
I can undo the const.



Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:4
+struct X {
+  explicit operator bool();
+};

JonasToth wrote:
> I didn't see a test that utilized this struct, did I overlook it?
> 
> Implicit conversion to `bool`-cases would be interesting as well. Maybe 
> implicit conversion to integral in general.
Leftover from copy/paste of readability-simplify-bool-expr.cpp, I'll remove it.

Implicit conversion cases are covered by the other file.  Here, I'm just 
testing the cases that interact with member variables because the matcher needs 
to use memberExpr() and not just declRefExpr()



Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:35
+  bool m_b3 = false;
+  bool m_b4 = false;
+  int *m_p = nullptr;

JonasToth wrote:
> Would bitfields with a single bit be of interest as well?
That could be an enhancement, but it's not addressed by this patch.



Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:160
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression 
result
+  // CHECK-FIXES: {{^  m_b2 = i <= 20;$}}
+

JonasToth wrote:
> For this case the logical inversion is simple an obviously `<=`, but what if 
> the condition is more complex?
> 
> I would personally prefer `!(i < 20)` instead + complex logical expressions 
> as additional test. They would be interesting for the `if` cases, too.
If you have complex conditions, you can run the check repeatedly until you 
reach a fixed point.

Using !(x) instead of the reverse operator could be added as an enhancement, 
but it is not covered by this patch.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56323



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

[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 5 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:4
+struct X {
+  explicit operator bool();
+};

JonasToth wrote:
> LegalizeAdulthood wrote:
> > JonasToth wrote:
> > > I didn't see a test that utilized this struct, did I overlook it?
> > > 
> > > Implicit conversion to `bool`-cases would be interesting as well. Maybe 
> > > implicit conversion to integral in general.
> > Leftover from copy/paste of readability-simplify-bool-expr.cpp, I'll remove 
> > it.
> > 
> > Implicit conversion cases are covered by the other file.  Here, I'm just 
> > testing the cases that interact with member variables because the matcher 
> > needs to use memberExpr() and not just declRefExpr()
> Which other file? The other revision?
I based this new file off readability-simplify-bool-expr.cpp that is already 
there.  It has all the main test cases for this check.



Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:160
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression 
result
+  // CHECK-FIXES: {{^  m_b2 = i <= 20;$}}
+

JonasToth wrote:
> LegalizeAdulthood wrote:
> > JonasToth wrote:
> > > For this case the logical inversion is simple an obviously `<=`, but what 
> > > if the condition is more complex?
> > > 
> > > I would personally prefer `!(i < 20)` instead + complex logical 
> > > expressions as additional test. They would be interesting for the `if` 
> > > cases, too.
> > If you have complex conditions, you can run the check repeatedly until you 
> > reach a fixed point.
> > 
> > Using !(x) instead of the reverse operator could be added as an 
> > enhancement, but it is not covered by this patch.
> But the current version is the enhancement.
> The complex condition, like `i < 20 && i > 0` will be the common case, so the 
> inversion (with parens) should always be used (`!(i < 20 && i > 0)`) and 
> specializations can be done later.
No, the current version is a bug fix.  The simplification of boolean 
expressions was already there, but it didn't handle member variables properly.

You're asking for an additional enhancement about the way boolean expressions 
are simplified.  That's fine for an additional enhancement of the check, but it 
is not the point of this patch.

The complex expression you're discussing in your comment is not changed by this 
change, nor is it changed by the existing check.


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

https://reviews.llvm.org/D56323



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

[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 180379.
LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added a comment.

Update from review comments


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

https://reviews.llvm.org/D56323

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  test/clang-tidy/readability-simplify-bool-expr-members.cpp

Index: test/clang-tidy/readability-simplify-bool-expr-members.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-members.cpp
@@ -0,0 +1,356 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+class A {
+public:
+  int m;
+};
+
+struct S {
+  S() : m_ar(s_a) {}
+
+  void operator_equals();
+  void operator_or();
+  void operator_and();
+  void ternary_operator();
+  void operator_not_equal();
+  void simple_conditional_assignment_statements();
+  void complex_conditional_assignment_statements();
+  void chained_conditional_assignment();
+  bool non_null_pointer_condition();
+  bool null_pointer_condition();
+  bool negated_non_null_pointer_condition();
+  bool negated_null_pointer_condition();
+  bool integer_not_zero();
+  bool member_pointer_nullptr();
+  bool integer_member_implicit_cast();
+  bool expr_with_cleanups();
+
+  bool m_b1 = false;
+  bool m_b2 = false;
+  bool m_b3 = false;
+  bool m_b4 = false;
+  int *m_p = nullptr;
+  int A::*m_m = nullptr;
+  int m_i = 0;
+  A *m_a = nullptr;
+  static A s_a;
+  A &m_ar;
+};
+
+void S::operator_equals() {
+  int i = 0;
+  m_b1 = (i > 2);
+  if (m_b1 == true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 5;
+  } else {
+i = 6;
+  }
+  m_b2 = (i > 4);
+  if (m_b2 == false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(!m_b2\) {$}}
+i = 7;
+  } else {
+i = 9;
+  }
+  m_b3 = (i > 6);
+  if (true == m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b3\) {$}}
+i = 10;
+  } else {
+i = 11;
+  }
+  m_b4 = (i > 8);
+  if (false == m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(!m_b4\) {$}}
+i = 12;
+  } else {
+i = 13;
+  }
+}
+
+void S::operator_or() {
+  int i = 0;
+  m_b1 = (i > 10);
+  if (m_b1 || false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 14;
+  } else {
+i = 15;
+  }
+  m_b2 = (i > 10);
+  if (m_b2 || true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(true\) {$}}
+i = 16;
+  } else {
+i = 17;
+  }
+  m_b3 = (i > 10);
+  if (false || m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b3\) {$}}
+i = 18;
+  } else {
+i = 19;
+  }
+  m_b4 = (i > 10);
+  if (true || m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(true\) {$}}
+i = 20;
+  } else {
+i = 21;
+  }
+}
+
+void S::operator_and() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (m_b1 && false) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(false\) {$}}
+i = 22;
+  } else {
+i = 23;
+  }
+  m_b2 = (i > 20);
+  if (m_b2 && true) {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b2\) {$}}
+i = 24;
+  } else {
+i = 25;
+  }
+  m_b3 = (i > 20);
+  if (false && m_b3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(false\) {$}}
+i = 26;
+  } else {
+i = 27;
+  }
+  m_b4 = (i > 20);
+  if (true && m_b4) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b4\) {$}}
+i = 28;
+  } else {
+i = 29;
+  }
+}
+
+void S::ternary_operator() {
+  int i = 0;
+  m_b1 = (i > 20) ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b1 = i > 20;$}}
+
+  m_b2 = (i > 20) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b2 = i <= 20;$}}
+
+  m_b3 = ((i > 20)) ? false : true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b3 = i <= 20;$}}
+}
+
+void S::operator_not_equal() {
+  int i = 0;
+  m_b1 = (i > 20);
+  if (false != m_b1) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{^  if \(m_b1\) {$}}
+i = 30;
+  } else {
+i = 31;
+  }
+  m_b2 = (i > 20);
+  if (true != m_b2) {
+// CHE

[PATCH] D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 10 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:386
 
-  bool BoolValue = Bool->getValue();
+  const bool BoolValue = Bool->getValue();
 

JonasToth wrote:
> `const` on values is uncommon in clang-tidy code. Please keep that consistent.
I can change the code, but I don't understand the push back.

"That's the way it's done elsewhere" just doesn't seem like good reasoning.

I write const on values to signify that they are computed once and only once.  
What is gained by removing that statement of once-and-only-once?



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:533-540
+  switchStmt(has(
+  compoundStmt(
+  has(defaultStmt(hasDescendant(ifStmt(hasThen(returnsBool(Value)),
+   unless(hasElse(stmt(
+.bind(CompoundIfId)))
+  .bind(DefaultId)),
+  has(returnStmt(has(cxxBoolLiteral(equals(!Value))

aaron.ballman wrote:
> The check duplication here is unfortunate -- can you factor out the 
> `hasDescendant()` bits into a variable that is reused, and perhaps use 
> `anyOf(caseStmt(stuff()), defaultStmt(stuff()))` rather than separate 
> functions?
I'm not a fan of duplication, either.

However, I have to know if it's a CaseStmt or DefaultStmt in the replacement 
code and that's tied to the id, so I'm not sure how to collapse it further.



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:719
+bool Negated, const SwitchCase *SwitchCase) {
+  assert(SwitchCase != nullptr);
+

aaron.ballman wrote:
> Add a message to the assertion (same with the other ones).
I'm not sure what you're asking for.  I based these asserts off the existing 
assert in the file.



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:743
+  // if the next statement after the if contains a return statement of
+  // the correct form.  Ideally we'd be able to express this with the
+  // matchers, but that is currently impossible.

JonasToth wrote:
> double space
What exactly is the problem?



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:745
+  // matchers, but that is currently impossible.
+  //
+  const auto *If = dyn_cast(SwitchCase->getSubStmt());

JonasToth wrote:
> redundant empty comment line
Meh, it's not redundant.  It's there to aid readability of a big block of text 
by visually separating it from the associated code.

Why is this a problem?



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:747
+  const auto *If = dyn_cast(SwitchCase->getSubStmt());
+  assert(If != nullptr);
+  const CXXBoolLiteralExpr *Lit = stmtReturnsBool(If, Negated);

JonasToth wrote:
> I think this assertion does not hold true from the matcher.
> The matcher requires only `hasDescendent(ifStmt())`, but that does not ensure 
> the first stmt is the `ifStmt`.
> 
> e.g.
> ```
> case 10: {
>   loggingCall();
>   if(foo) ...
> ```
> 
> Is this excluded?
> }
Look more carefully at the AST.  CaseStmt has exactly one child.  That child 
can either be a compound statement block (which was already correctly handled 
by the check) or it can be a single statement.  This change enhances the check 
to handle the single statement child of the CaseStmt and DefaultStmt.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56303



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

[PATCH] D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 180380.
LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.

Update from review comments


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

https://reviews.llvm.org/D56303

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tidy/readability/SimplifyBooleanExprCheck.h
  test/clang-tidy/readability-simplify-bool-expr-case.cpp

Index: test/clang-tidy/readability-simplify-bool-expr-case.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-case.cpp
@@ -0,0 +1,586 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+bool switch_stmt(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+if (b == true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 1:
+if (b == false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(!b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 2:
+if (b && true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 3:
+if (b && false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(false\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 4:
+if (b || true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(true\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+// CHECK-FIXES-NEXT: {{break;}}
+
+  case 5:
+if (b || false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 6:
+return i > 0 ? true : false;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: {{return i > 0;}}
+
+  case 7:
+return i > 0 ? false : true;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: {{return i <= 0;}}
+
+  case 8:
+if (true)
+  j = 10;
+else
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+// CHECK-FIXES:  {{j = 10;$}}
+// CHECK-FIXES-NEXT: {{break;$}}
+
+  case 9:
+if (false)
+  j = -20;
+else
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+// CHECK-FIXES: {{j = 10;}}
+// CHECK-FIXES-NEXT: {{break;}}
+
+  case 10:
+if (j > 10)
+  return true;
+else
+  return false;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{return j > 10;}}
+
+  case 11:
+if (j > 10)
+  return false;
+else
+  return true;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{return j <= 10;}}
+
+  case 12:
+if (j > 10)
+  b = true;
+else
+  b = false;
+return b;
+// CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{b = j > 10;}}
+// CHECK-FIXES-NEXT: {{return b;}}
+
+  case 13:
+if (j > 10)
+  b = false;
+else
+  b = true;
+return b;
+// CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{b = j <= 10;}}
+// CHECK-FIXES-NEXT: {{return b;}}
+
+  case 14:
+if (j > 10)
+  return true;
+return false;
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+// FIXES: {{return j > 10;}}
+
+  case 15:
+if (j > 10)
+  return false;
+return true;
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+// FIXES: {{return j <= 10;}}
+
+  case 16:
+if (j > 10)
+  return true;
+return true;
+return false;
+
+  case 17:
+if (j > 10)
+  return false;
+return false;
+return true;
+
+  case 100: {
+if (b == true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+  }
+
+  case 101: {
+if (b == false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(!b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+  }
+
+  case 102: {
+if (b && true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(

[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

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

In D56323#1347450 , @JonasToth wrote:

> Did you check if the changes actually fix the problematic behaviour? Did you 
> run it over any project and did the code-transformation break the code, false 
> positives, ...?
>
> Otherwise LGTM


The test cases that I added covered the behavior described in the bug report.  
I didn't run it over clang to see if it introduced any false positives, I can 
do that.  However, when I originally wrote this check it was pretty well tested 
and I have confidence that I haven't regressed. However, it's still worth 
testing, so I will do that.


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

https://reviews.llvm.org/D56323



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

[PATCH] D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 180384.
LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.

Update from review comments


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

https://reviews.llvm.org/D56303

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tidy/readability/SimplifyBooleanExprCheck.h
  test/clang-tidy/readability-simplify-bool-expr-case.cpp

Index: test/clang-tidy/readability-simplify-bool-expr-case.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-case.cpp
@@ -0,0 +1,586 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+bool switch_stmt(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+if (b == true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 1:
+if (b == false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(!b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 2:
+if (b && true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 3:
+if (b && false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(false\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 4:
+if (b || true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(true\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+// CHECK-FIXES-NEXT: {{break;}}
+
+  case 5:
+if (b || false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 6:
+return i > 0 ? true : false;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: {{return i > 0;}}
+
+  case 7:
+return i > 0 ? false : true;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: {{return i <= 0;}}
+
+  case 8:
+if (true)
+  j = 10;
+else
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+// CHECK-FIXES:  {{j = 10;$}}
+// CHECK-FIXES-NEXT: {{break;$}}
+
+  case 9:
+if (false)
+  j = -20;
+else
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+// CHECK-FIXES: {{j = 10;}}
+// CHECK-FIXES-NEXT: {{break;}}
+
+  case 10:
+if (j > 10)
+  return true;
+else
+  return false;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{return j > 10;}}
+
+  case 11:
+if (j > 10)
+  return false;
+else
+  return true;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{return j <= 10;}}
+
+  case 12:
+if (j > 10)
+  b = true;
+else
+  b = false;
+return b;
+// CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{b = j > 10;}}
+// CHECK-FIXES-NEXT: {{return b;}}
+
+  case 13:
+if (j > 10)
+  b = false;
+else
+  b = true;
+return b;
+// CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{b = j <= 10;}}
+// CHECK-FIXES-NEXT: {{return b;}}
+
+  case 14:
+if (j > 10)
+  return true;
+return false;
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+// FIXES: {{return j > 10;}}
+
+  case 15:
+if (j > 10)
+  return false;
+return true;
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+// FIXES: {{return j <= 10;}}
+
+  case 16:
+if (j > 10)
+  return true;
+return true;
+return false;
+
+  case 17:
+if (j > 10)
+  return false;
+return false;
+return true;
+
+  case 100: {
+if (b == true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+  }
+
+  case 101: {
+if (b == false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(!b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+  }
+
+  case 102: {
+if (b && true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(

[PATCH] D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 3 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:719
+bool Negated, const SwitchCase *SwitchCase) {
+  assert(SwitchCase != nullptr);
+

riccibruno wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > Add a message to the assertion (same with the other ones).
> > I'm not sure what you're asking for.  I based these asserts off the 
> > existing assert in the file.
> Something like `assert((SwitchCase != nullptr) && "Some appropriate message 
> blablabla!");`
> Keep the parentheses around the operands of `!=` even if they are not 
> strictly needed
> since otherwise some bots will complain.
OK, I can do that; I wasn't sure if it was being suggested that I use some 
custom assert macro that took the message as a parameter.


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

https://reviews.llvm.org/D56303



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

[PATCH] D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 180385.
LegalizeAdulthood added a comment.

No really, update from review comments `:)`


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

https://reviews.llvm.org/D56303

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tidy/readability/SimplifyBooleanExprCheck.h
  test/clang-tidy/readability-simplify-bool-expr-case.cpp

Index: test/clang-tidy/readability-simplify-bool-expr-case.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-simplify-bool-expr-case.cpp
@@ -0,0 +1,586 @@
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+
+bool switch_stmt(int i, int j, bool b) {
+  switch (i) {
+  case 0:
+if (b == true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 1:
+if (b == false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(!b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 2:
+if (b && true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+
+  case 3:
+if (b && false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(false\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 4:
+if (b || true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(true\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+// CHECK-FIXES-NEXT: {{break;}}
+
+  case 5:
+if (b || false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+
+  case 6:
+return i > 0 ? true : false;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: {{return i > 0;}}
+
+  case 7:
+return i > 0 ? false : true;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: {{.*}} in ternary expression result
+// CHECK-FIXES: {{return i <= 0;}}
+
+  case 8:
+if (true)
+  j = 10;
+else
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+// CHECK-FIXES:  {{j = 10;$}}
+// CHECK-FIXES-NEXT: {{break;$}}
+
+  case 9:
+if (false)
+  j = -20;
+else
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in if statement condition
+// CHECK-FIXES: {{j = 10;}}
+// CHECK-FIXES-NEXT: {{break;}}
+
+  case 10:
+if (j > 10)
+  return true;
+else
+  return false;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{return j > 10;}}
+
+  case 11:
+if (j > 10)
+  return false;
+else
+  return true;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} in conditional return statement
+// CHECK-FIXES: {{return j <= 10;}}
+
+  case 12:
+if (j > 10)
+  b = true;
+else
+  b = false;
+return b;
+// CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{b = j > 10;}}
+// CHECK-FIXES-NEXT: {{return b;}}
+
+  case 13:
+if (j > 10)
+  b = false;
+else
+  b = true;
+return b;
+// CHECK-MESSAGES: :[[@LINE-4]]:11: warning: {{.*}} in conditional assignment
+// CHECK-FIXES: {{b = j <= 10;}}
+// CHECK-FIXES-NEXT: {{return b;}}
+
+  case 14:
+if (j > 10)
+  return true;
+return false;
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+// FIXES: {{return j > 10;}}
+
+  case 15:
+if (j > 10)
+  return false;
+return true;
+// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: {{.*}} in conditional return
+// FIXES: {{return j <= 10;}}
+
+  case 16:
+if (j > 10)
+  return true;
+return true;
+return false;
+
+  case 17:
+if (j > 10)
+  return false;
+return false;
+return true;
+
+  case 100: {
+if (b == true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j = 10;}}
+  }
+
+  case 101: {
+if (b == false)
+  j = -20;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(!b\)}}
+// CHECK-FIXES-NEXT: {{j = -20;}}
+  }
+
+  case 102: {
+if (b && true)
+  j = 10;
+break;
+// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: {{.*}} to boolean operator
+// CHECK-FIXES: {{if \(b\)}}
+// CHECK-FIXES-NEXT: {{j 

[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Uh... I'm developing on Windows, which doesn't have a solution for generating 
compile_commands.json, so I can't easily test this on the entire clang 
codebase.  I will test on a source file based on the original bug report.


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

https://reviews.llvm.org/D56323



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

[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I managed to do some limited testing on the original source file from the bug 
report in lldb and verified that the correct fix was applied there.  I also 
tried a few other files and verified no false positives.


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

https://reviews.llvm.org/D56323



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

[PATCH] D45932: [clang-tidy][modernize-raw-string-literal] Don't replace upper ASCII with raw literals

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: test/clang-tidy/modernize-raw-string-literal.cpp:44
+char const *const MultibyteSnowman("\xE2\x98\x83");
+// CHECK-FIXES: {{^}}char const *const MultibyteSnowman("\xE2\x98\x83");{{$}}
 

IIRC, the default behavior is that if no matching CHECK-FIXES line is found, 
then it is considered a failure.  Have you tried your test code without your 
change to verify that this is the case?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45932



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

[PATCH] D45932: [clang-tidy][modernize-raw-string-literal] Don't replace upper ASCII with raw literals

[Richard via Phabricator via cfe-commits]

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

Other than a few minor nits, ship it.




Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:110
+
+  // Upper ASCII are disallowed too.
+  for (unsigned char C = 0xFFu; C >= 0x80u; --C)

"Upper ASCII" is a misnomer.  ASCII only defines character codes 0-127.  
Non-ASCII codes are what is being described here.



Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:111
+  // Upper ASCII are disallowed too.
+  for (unsigned char C = 0xFFu; C >= 0x80u; --C)
+DisallowedChars.set(C);

Why does this loop go down instead of up?  I would have expected a more 
conventional

for (unsigned char C = 0x80u; C <= 0xFFu; ++C)



Comment at: clang-tidy/modernize/RawStringLiteralCheck.h:39
   std::string DelimiterStem;
+  std::bitset<1 << CHAR_BIT> DisallowedChars;
   const bool ReplaceShorterLiterals;

Consider introducing a typedef for this ugly type name.



Comment at: test/clang-tidy/modernize-raw-string-literal.cpp:44
+char const *const MultibyteSnowman("\xE2\x98\x83");
+// CHECK-FIXES: {{^}}char const *const MultibyteSnowman("\xE2\x98\x83");{{$}}
 

Consider adding CHECK-FIXES lines for the other test cases that should remain 
unmolested by the check.


https://reviews.llvm.org/D45932



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

[PATCH] D45932: [clang-tidy][modernize-raw-string-literal] Don't replace upper ASCII with raw literals

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:111
+  // Upper ASCII are disallowed too.
+  for (unsigned char C = 0xFFu; C >= 0x80u; --C)
+DisallowedChars.set(C);

zinovy.nis wrote:
> LegalizeAdulthood wrote:
> > Why does this loop go down instead of up?  I would have expected a more 
> > conventional
> > 
> > for (unsigned char C = 0x80u; C <= 0xFFu; ++C)
> The only reason is that `++C` for `C==0xFFu` is `0` so `C <= 0xFF` is always 
> satisfied leading to inifinite loop.
OK, makes sense.  I rarely write loops that iterate over character types.


https://reviews.llvm.org/D45932



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

[PATCH] D81336: [clang-tidy] simplify-bool-expr ignores template instantiations

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Yeah, the restriction to the header file is a bug.  It was a misconception on 
my part when I originally wrote the matcher.

See https://bugs.llvm.org/show_bug.cgi?id=26332 which should also be fixed by 
the change here.

I had attempted to make progress on fixing that bug (26332) but some retooling 
of the testing framework is needed to validate
that the **header** has a fixit applied to it.  I made some progress by 
refactoring the python script, but as usual it got bogged
down in clang review hell and I haven't had time to make any more forward 
progress on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81336



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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D128402

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

[PATCH] D128157: [clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I pushed this change, but for some reason phabricator doesn't show it and close 
the review... I wonder if it's because I rebased it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128157

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
This revision now requires changes to proceed.

I can submit after you address additional comments by Nathan James.


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

https://reviews.llvm.org/D128402

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

[PATCH] D128715: [clang-tidy] Fix confusable identifiers interaction with DeclContext

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc/confusable-identifiers.cpp:43
+  return q0 < q1;
+}

aaron.ballman wrote:
> It looks like there's quite a bit of missing test coverage for all the 
> various situations in C++. Here are some additional test cases to add:
> ```
> template 
> struct S {
>   template  // Should warn on this one, right?
>   void func2() {}
> };
> 
> template 
> void func(int il) { // Should warn on this one, right?
> }
> 
> template 
> void other() {
>   int OO = 0; // Should warn on this one, right?
> }
> int OO = 0; // But not this one
> 
> namespace foo {
>   int i1;
> }
> 
> namespace bar {
>   int il;  // Should not warn about this
> }
> 
> namespace foo {
>   int il; // But should warn about this
> }
> 
> struct Base {
>   virtual void O0();
> 
> private:
>   void II();
> };
> 
> struct Derived : Base {
>   void OO(); // We should warn about this
>   void I1(); // But not this
> };
> ```
> (I'm sure there are more that I've missed, but you get the idea.) If any of 
> these point out other issues, feel free to address them in a follow-up patch 
> if you'd prefer.
Another case to consider:
```
template 
void f();
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128715

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

[PATCH] D128697: [clang-tidy] Add new check `bugprone-unhandled-exception-at-sto`

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

This whole check seems weird to me.  I mean, almost every use of a standard 
container could throw `std::bad_alloc` but we don't insist on a local `catch` 
for `bad_alloc` at every possible throwing call site.

Why would we assume that every call site of `stoi` or `stod` **must** have an 
exception handler immediately around it?  It's perfectly acceptable for an 
application to handle this at an outer scope that can't be detected by 
clang-tidy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128697

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

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
This revision now requires changes to proceed.

Please add a summary of the fix to the release notes for this check.


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

https://reviews.llvm.org/D127293

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

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
This revision now requires changes to proceed.

Clang-tidy tests and docs have been moved to subdirectories by module, please 
rebase to `main:HEAD`


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

https://reviews.llvm.org/D54943

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I can submit after you address additional comments by Nathan James.


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

https://reviews.llvm.org/D128402

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Please provide the name and email address you wish to use on the commit and I 
will submit.


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

https://reviews.llvm.org/D128402

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

[PATCH] D127807: [clang-tidy] Properly forward clang-tidy output when running tests

[Richard via Phabricator via cfe-commits]

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

Thanks for this, like Nathan James this has been pestering me for a while `:)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127807

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

[PATCH] D128511: [clang-tidy] Make the cert/uppercase-literal-suffix-integer fully hermetic.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/integral_constant.h:1
+#ifndef _INTEGRAL_CONSTANT_H_
+#define _INTEGRAL_CONSTANT_H_

Identifiers beginning with `_` followed by an uppercase letter are [[ 
https://en.cppreference.com/w/cpp/language/identifiers | reserved for the 
implementation ]].

Include guards should never include double underscores or begin with an 
underscore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128511

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

[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:237
 
+- Improved :doc:`bugprone-signal-handler
+  ` check. Partial

sort by check name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118996

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

[PATCH] D128157: [clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D128157#3612741 , @myhsu wrote:

> You forgot to add 
> 
>  "Differential Revison: https://reviews.llvm.org/DXX"; in the commit 
> message, which is how Phabricator identifies the differential to close. I 
> don't think it has anything to do with rebasing.

Oh, OK, that makes sense, oops.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128157

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

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:131
+- New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
+  ` check.
+

This link is wrong, please install Sphinx and build the target 
`docs-clang-tools-html` to validate links


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:130
 
+- New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
+  ` check.

Sort new checks by check name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa65a3bffd31f: [clang-tidy] Don't treat invalid branches 
as identical (authored by ishaangandhi, committed by LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128402

Files:
  clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp
@@ -0,0 +1,9 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s bugprone-branch-clone %t
+
+int test_unknown_expression() {
+  if (unknown_expression_1) {// CHECK-MESSAGES: :[[@LINE]]:7: error: 
use of undeclared identifier 'unknown_expression_1' [clang-diagnostic-error]
+function1(unknown_expression_2); // CHECK-MESSAGES: :[[@LINE]]:15: error: 
use of undeclared identifier 'unknown_expression_2' [clang-diagnostic-error]
+  } else {
+function2(unknown_expression_3); // CHECK-MESSAGES: :[[@LINE]]:15: error: 
use of undeclared identifier 'unknown_expression_3' [clang-diagnostic-error]
+  }
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -158,6 +158,10 @@
 - Fixed nonsensical suggestion of :doc:`altera-struct-pack-align
   ` check for empty structs.
 
+- Fixed a false positive in :doc:`bugprone-branch-clone
+  ` when the branches
+  involve unknown expressions.
+
 - Fixed some false positives in :doc:`bugprone-infinite-loop
   ` involving dependent expressions.
 
Index: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -19,6 +19,17 @@
 /// Returns true when the statements are Type I clones of each other.
 static bool areStatementsIdentical(const Stmt *LHS, const Stmt *RHS,
const ASTContext &Context) {
+  if (isa(LHS) && isa(RHS)) {
+// If we have errors in expressions, we will be unable
+// to accurately profile and compute hashes for each
+// of the left and right statements.
+const auto *LHSExpr = llvm::cast(LHS);
+const auto *RHSExpr = llvm::cast(RHS);
+if (LHSExpr->containsErrors() && RHSExpr->containsErrors()) {
+  return false;
+}
+  }
+
   llvm::FoldingSetNodeID DataLHS, DataRHS;
   LHS->Profile(DataLHS, Context, false);
   RHS->Profile(DataRHS, Context, false);


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp
@@ -0,0 +1,9 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s bugprone-branch-clone %t
+
+int test_unknown_expression() {
+  if (unknown_expression_1) {// CHECK-MESSAGES: :[[@LINE]]:7: error: use of undeclared identifier 'unknown_expression_1' [clang-diagnostic-error]
+function1(unknown_expression_2); // CHECK-MESSAGES: :[[@LINE]]:15: error: use of undeclared identifier 'unknown_expression_2' [clang-diagnostic-error]
+  } else {
+function2(unknown_expression_3); // CHECK-MESSAGES: :[[@LINE]]:15: error: use of undeclared identifier 'unknown_expression_3' [clang-diagnostic-error]
+  }
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -158,6 +158,10 @@
 - Fixed nonsensical suggestion of :doc:`altera-struct-pack-align
   ` check for empty structs.
 
+- Fixed a false positive in :doc:`bugprone-branch-clone
+  ` when the branches
+  involve unknown expressions.
+
 - Fixed some false positives in :doc:`bugprone-infinite-loop
   ` involving dependent expressions.
 
Index: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -19,6 +19,17 @@
 /// Returns true when the statements are Type I clones of each other.
 static bool areStatementsIdentical(const Stmt *LHS, const Stmt *RHS,
const ASTContext &Context) {
+  if (isa(LHS) && isa(RHS)) {
+// If we have errors in expressions, we will be unable
+// to accurately profile and c

[PATCH] D127807: [clang-tidy] Properly forward clang-tidy output when running tests

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I will submit it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127807

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

[PATCH] D127807: [clang-tidy] Properly forward clang-tidy output when running tests

[Richard via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf7a80c3d08d4: [clang-tidy] Properly forward clang-tidy 
output when running tests (authored by nicovank, committed by 
LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127807

Files:
  clang-tools-extra/test/clang-tidy/check_clang_tidy.py


Index: clang-tools-extra/test/clang-tidy/check_clang_tidy.py
===
--- clang-tools-extra/test/clang-tidy/check_clang_tidy.py
+++ clang-tools-extra/test/clang-tidy/check_clang_tidy.py
@@ -173,13 +173,13 @@
 print('Running ' + repr(args) + '...')
 clang_tidy_output = try_run(args)
 print(' clang-tidy output ---')
-print(clang_tidy_output.encode())
-
print('\n--')
+print(clang_tidy_output.encode(sys.stdout.encoding, 
errors="replace").decode(sys.stdout.encoding))
+print('--')
 
 diff_output = try_run(['diff', '-u', self.original_file_name, 
self.temp_file_name], False)
-print('-- Fixes 
-\n' +
-  diff_output +
-  
'\n--')
+print('-- Fixes -')
+print(diff_output)
+print('--')
 return clang_tidy_output
 
   def check_fixes(self):


Index: clang-tools-extra/test/clang-tidy/check_clang_tidy.py
===
--- clang-tools-extra/test/clang-tidy/check_clang_tidy.py
+++ clang-tools-extra/test/clang-tidy/check_clang_tidy.py
@@ -173,13 +173,13 @@
 print('Running ' + repr(args) + '...')
 clang_tidy_output = try_run(args)
 print(' clang-tidy output ---')
-print(clang_tidy_output.encode())
-print('\n--')
+print(clang_tidy_output.encode(sys.stdout.encoding, errors="replace").decode(sys.stdout.encoding))
+print('--')
 
 diff_output = try_run(['diff', '-u', self.original_file_name, self.temp_file_name], False)
-print('-- Fixes -\n' +
-  diff_output +
-  '\n--')
+print('-- Fixes -')
+print(diff_output)
+print('--')
 return clang_tidy_output
 
   def check_fixes(self):
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47
+CRLF,
+CRLFCR,
+  };

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > I'm a bit confused by this one as this is not a valid line ending 
> > > > > (it's either three valid line endings or two valid line endings, 
> > > > > depending on how you look at it). Can you explain why this is needed?
> > > > It's a state machine, where the states are named for what we've seen so 
> > > > far and we're looking for //two// consecutive line endings, not just 
> > > > one.  Does it make sense now?
> > > Thanks, I understood it was a state machine, but it's a confused one to 
> > > me. `\r` was the line ending on Mac Classic, I've not seen it used 
> > > outside of that platform (and I've not seen anyone write code for that 
> > > platform in a long time). So, to me, the only valid combinations of line 
> > > endings to worry about are: `LF LF`; `CRLF CRLF`; `CRLF LF`; `LF CRLF`.
> > > 
> > > `LF LF` returns false (Nothing -> LF -> return false)
> > > `CRLF CRLF` returns false (Nothing -> CR -> CRLF -> CRLFCR -> return 
> > > false)
> > > `CRLF LF` returns true (Nothing -> CR -> CRLF -> LF -> finish loop)
> > > `LF CRLF` returns true (Nothing -> LF -> CR -> CRLF -> finish loop)
> > > 
> > > (If you also intend to support Mac Classic line endings for some reason, 
> > > this gets even more complicated.)
> > I was trying to follow "be liberal in what you accept as input and 
> > conservative in what you generate as output" maxim.  I can remove the `CR` 
> > as a line ending case if you think it's too obscure.
> If Clang supports it as a line ending, we probably should too, but... how do 
> we handle CRLF vs "I mixed a CR with an LF by accident" kind of inputs? 
> (Maybe we just treat that as CRLF and if the behavior is bad, the user 
> shouldn't mix their line endings that way; I think that's defensible.) That 
> seems to be similar to the scenario that's confusing me above where the user 
> mixed an LF and CRLF by accident.
Well, as far as Clang is concerned it's all just "whitespace" that gets eaten 
up by the preprocessor.  Actually, that gives me a thought.  A preprocessing 
directive is considered to end at the physical line ending, so I should look to 
see what sort of characters it considers to "end the line".

For the accidental mix-up, I'm not going to worry about that here.  Your input 
files are assumed to be "well formed".  The worst that happens in this check is 
that two blocks of macros that //look// like they are separated by a blank line 
are considered as a single clump by this check.

In other words, the worst that can happen is:
  - Two clumps of macros are considered together.
  - One clump of macros that is discarded because it doesn't follow the 
constraints "taints" an adjacent clump of macros that do follow the constraints.

Either way, nothing harmful happens to your code.  It will still compile and be 
syntactically and semantically equivalent to what was there before.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:67-68
+
+// Undefining a macro invalidates adjacent macros
+// from being considered as an enum.
+#define REMOVED1 1

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > LegalizeAdulthood wrote:
> > > > > aaron.ballman wrote:
> > > > > > What about an #undef that's not adjacent to any macros? e.g.,
> > > > > > ```
> > > > > > #define FOO 1
> > > > > > #define BAR 2
> > > > > > #define BAZ 3
> > > > > > 
> > > > > > int i = 12;
> > > > > > 
> > > > > > #if defined(FROBBLE)
> > > > > > #undef FOO
> > > > > > #endif
> > > > > > ```
> > > > > > I'm worried that perhaps other code elsewhere will be checking 
> > > > > > `defined(FOO)` perhaps in cases conditionally compiled away, and 
> > > > > > switching `FOO` to be an enum constant will break other 
> > > > > > configurations. To be honest, I'm a bit worried about that for all 
> > > > > > of the transformations here... and I don't know a good way to 
> > > > > > address that aside from "don't use the check". It'd be interesting 
> > > > > > to know what kind of false positive rate we have for the fixes if 
> > > > > > we ran it over a large corpus of code.
> > > > > Yeah, the problem arises whenever you make any changes to a header 
> > > > > file.  Did you also change all translation units that include the 
> > > > > header?  What about conditionally compiled code that was "off" in the 
> > > > > translation unit for the automated change?  Currently, we don't have 
> > > > > a way of analyzing a group of translation units together for a 
> > > > > cohesive change, nor do we have any way of inspecting more deeply 
> > > > > into conditionally compiled cod

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 3 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47
+CRLF,
+CRLFCR,
+  };

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > LegalizeAdulthood wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > I'm a bit confused by this one as this is not a valid line ending 
> > > > > > > (it's either three valid line endings or two valid line endings, 
> > > > > > > depending on how you look at it). Can you explain why this is 
> > > > > > > needed?
> > > > > > It's a state machine, where the states are named for what we've 
> > > > > > seen so far and we're looking for //two// consecutive line endings, 
> > > > > > not just one.  Does it make sense now?
> > > > > Thanks, I understood it was a state machine, but it's a confused one 
> > > > > to me. `\r` was the line ending on Mac Classic, I've not seen it used 
> > > > > outside of that platform (and I've not seen anyone write code for 
> > > > > that platform in a long time). So, to me, the only valid combinations 
> > > > > of line endings to worry about are: `LF LF`; `CRLF CRLF`; `CRLF LF`; 
> > > > > `LF CRLF`.
> > > > > 
> > > > > `LF LF` returns false (Nothing -> LF -> return false)
> > > > > `CRLF CRLF` returns false (Nothing -> CR -> CRLF -> CRLFCR -> return 
> > > > > false)
> > > > > `CRLF LF` returns true (Nothing -> CR -> CRLF -> LF -> finish loop)
> > > > > `LF CRLF` returns true (Nothing -> LF -> CR -> CRLF -> finish loop)
> > > > > 
> > > > > (If you also intend to support Mac Classic line endings for some 
> > > > > reason, this gets even more complicated.)
> > > > I was trying to follow "be liberal in what you accept as input and 
> > > > conservative in what you generate as output" maxim.  I can remove the 
> > > > `CR` as a line ending case if you think it's too obscure.
> > > If Clang supports it as a line ending, we probably should too, but... how 
> > > do we handle CRLF vs "I mixed a CR with an LF by accident" kind of 
> > > inputs? (Maybe we just treat that as CRLF and if the behavior is bad, the 
> > > user shouldn't mix their line endings that way; I think that's 
> > > defensible.) That seems to be similar to the scenario that's confusing me 
> > > above where the user mixed an LF and CRLF by accident.
> > Well, as far as Clang is concerned it's all just "whitespace" that gets 
> > eaten up by the preprocessor.  Actually, that gives me a thought.  A 
> > preprocessing directive is considered to end at the physical line ending, 
> > so I should look to see what sort of characters it considers to "end the 
> > line".
> > 
> > For the accidental mix-up, I'm not going to worry about that here.  Your 
> > input files are assumed to be "well formed".  The worst that happens in 
> > this check is that two blocks of macros that //look// like they are 
> > separated by a blank line are considered as a single clump by this check.
> > 
> > In other words, the worst that can happen is:
> >   - Two clumps of macros are considered together.
> >   - One clump of macros that is discarded because it doesn't follow the 
> > constraints "taints" an adjacent clump of macros that do follow the 
> > constraints.
> > 
> > Either way, nothing harmful happens to your code.  It will still compile 
> > and be syntactically and semantically equivalent to what was there before.
> > 
> > Actually, that gives me a thought. A preprocessing directive is considered 
> > to end at the physical line ending, so I should look to see what sort of 
> > characters it considers to "end the line".
> 
> All of `\r`, `\n`, `\r\n` I believe (you can double-check in 
> `Lexer::LexTokenInternal()`
> 
> > Either way, nothing harmful happens to your code. It will still compile and 
> > be syntactically and semantically equivalent to what was there before.
> 
> Oh, that's a very good point, thank you. I think that's reasonable fallback 
> behavior for these weird edge cases.
Well. maybe.

If you look at `Lexer::ReadToEndOfLine` which is used to skip to the end of a 
preprocessor directive you'll see that it considers the first of `'\r'`, `'\n'` 
or `'\0'` (end of file) as the end of the "line".  This is around line 2835 of 
Lexer.cpp in my tree.


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

https://reviews.llvm.org/D117522

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

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 418112.
LegalizeAdulthood added a comment.

- clang-format


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

https://reviews.llvm.org/D117522

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum2.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum3.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -0,0 +1,239 @@
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum
+// C++14 or later required for binary literals.
+
+#if 1
+#include "modernize-macro-to-enum.h"
+
+// These macros are skipped due to being inside a conditional compilation block.
+#define GOO_RED 1
+#define GOO_GREEN 2
+#define GOO_BLUE 3
+
+#endif
+
+#define RED 0xFF
+#define GREEN 0x00FF00
+#define BLUE 0xFF
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum [modernize-macro-to-enum]
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'RED' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GREEN' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BLUE' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: RED = 0xFF,
+// CHECK-FIXES-NEXT: GREEN = 0x00FF00,
+// CHECK-FIXES-NEXT: BLUE = 0xFF
+// CHECK-FIXES-NEXT: };
+
+// Verify that comments are preserved.
+#define CoordModeOrigin 0   /* relative to the origin */
+#define CoordModePrevious   1   /* relative to previous point */
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModeOrigin' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModePrevious' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: CoordModeOrigin = 0,   /* relative to the origin */
+// CHECK-FIXES-NEXT: CoordModePrevious =   1   /* relative to previous point */
+// CHECK-FIXES-NEXT: };
+
+// Verify that multiline comments are preserved.
+#define BadDrawable 9   /* parameter not a Pixmap or Window */
+#define BadAccess   10  /* depending on context:
+- key/button already grabbed
+- attempt to free an illegal 
+  cmap entry 
+- attempt to store into a read-only 
+  color map entry. */
+// - attempt to modify the access control
+//   list from other than the local host.
+//
+#define BadAlloc11  /* insufficient resources */
+// CHECK-MESSAGES: :[[@LINE-11]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadDrawable' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadAccess' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BadAlloc' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: BadDrawable = 9,   /* parameter not a Pixmap or Window */
+// CHECK-FIXES-NEXT: BadAccess =   10,  /* depending on context:
+// CHECK-FIXES-NEXT: - key/button already grabbed
+// CHECK-FIXES-NEXT: - attempt to free an illegal 
+// CHECK-FIXES-NEXT:   cmap entry 
+// CHECK-FIXES-NEXT: - attempt to store into a read-only 
+// CHECK-FIXES-NEXT:   color map entry. */
+// CHECK-FIXES-NEXT: // - attempt to modify the access control
+// CHECK-FIXES-NEXT: //   list from other than the

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG39b80c8380c8: [clang-tidy] Add modernize-macro-to-enum check 
(authored by LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117522

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum2.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum3.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -0,0 +1,239 @@
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum
+// C++14 or later required for binary literals.
+
+#if 1
+#include "modernize-macro-to-enum.h"
+
+// These macros are skipped due to being inside a conditional compilation block.
+#define GOO_RED 1
+#define GOO_GREEN 2
+#define GOO_BLUE 3
+
+#endif
+
+#define RED 0xFF
+#define GREEN 0x00FF00
+#define BLUE 0xFF
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum [modernize-macro-to-enum]
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'RED' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GREEN' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BLUE' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: RED = 0xFF,
+// CHECK-FIXES-NEXT: GREEN = 0x00FF00,
+// CHECK-FIXES-NEXT: BLUE = 0xFF
+// CHECK-FIXES-NEXT: };
+
+// Verify that comments are preserved.
+#define CoordModeOrigin 0   /* relative to the origin */
+#define CoordModePrevious   1   /* relative to previous point */
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModeOrigin' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModePrevious' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: CoordModeOrigin = 0,   /* relative to the origin */
+// CHECK-FIXES-NEXT: CoordModePrevious =   1   /* relative to previous point */
+// CHECK-FIXES-NEXT: };
+
+// Verify that multiline comments are preserved.
+#define BadDrawable 9   /* parameter not a Pixmap or Window */
+#define BadAccess   10  /* depending on context:
+- key/button already grabbed
+- attempt to free an illegal 
+  cmap entry 
+- attempt to store into a read-only 
+  color map entry. */
+// - attempt to modify the access control
+//   list from other than the local host.
+//
+#define BadAlloc11  /* insufficient resources */
+// CHECK-MESSAGES: :[[@LINE-11]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadDrawable' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadAccess' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BadAlloc' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: BadDrawable = 9,   /* parameter not a Pixmap or Window */
+// CHECK-FIXES-NEXT: BadAccess =   10,  /* depending on context:
+// CHECK-FIXES-NEXT: - key/button already grabbed
+// CHECK-FIXES-NEXT: - attempt to free an illegal 
+// CHECK-FIXES-NEXT:   cmap entry 
+// CHECK-FIXES-NEXT: - attempt to store into a read-only 
+// CHECK-FIXES-NEXT:   color map entry. */
+// CHECK-FIXES-NEXT: // - a

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47
+CRLF,
+CRLFCR,
+  };

aaron.ballman wrote:
> I'm a bit confused by this one as this is not a valid line ending (it's 
> either three valid line endings or two valid line endings, depending on how 
> you look at it). Can you explain why this is needed?
It's a state machine, where the states are named for what we've seen so far and 
we're looking for //two// consecutive line endings, not just one.  Does it make 
sense now?



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:83-84
+  const char *End = Begin + Token.getLength();
+  return std::none_of(
+  Begin, End, [](char C) { return C == '.' || std::toupper(C) == 'E'; });
+}

aaron.ballman wrote:
> Won't this cause a problem for hex literals like `0xE`?
Good catch, I'll add a test.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:106
+  int ConditionScopes;
+  unsigned int LastLine;
+  IncludeGuard GuardScanner;

aaron.ballman wrote:
> Maybe not worth worrying about, but should this be a `uint64_t` to handle 
> massive source files?
I use the type returned by getSpellingLineNumber.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:315
+  if (Info->isFunctionLike() || Info->isBuiltinMacro() ||
+  Info->tokens().empty() || Info->tokens().size() > 2)
+return;

aaron.ballman wrote:
> We don't seem to have any tests for literal suffixes and I *think* those will 
> show up here as additional tokens after the literal. e.g., `#define FOO 
> +1ULL`, so I think the size check here may be a problem.
On an earlier iteration I had some tests for literals with suffixes and the 
suffix is included in the literal token.  I can add some test cases just to 
make it clear what the behavior is when they are encountered.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:318
+
+  // It can be +Lit, -Lit or just Lit.
+  Token Tok = Info->tokens().front();

aaron.ballman wrote:
> It's worth pointing out that both of these are expressions that operate on a 
> literal, and if we're going to allow expressions that operator on a literal, 
> why only these two? e.g. why not allow `#define FOO ~0U` or `#define BAR FOO 
> + 1`? Someday (not today), it would be nice for this to work on any 
> expression that's a valid constant expression.
A later enhancement can generalize to literal expressions (which are valid 
initializers for an enumeration), but I wanted to cover the most common case of 
simple negative integers in this first pass.



Comment at: clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp:88-92
+CheckFactories.registerCheck(
+"modernize-use-equals-default");
 CheckFactories.registerCheck(
 "modernize-use-equals-delete");
+CheckFactories.registerCheck("modernize-use-nodiscard");

aaron.ballman wrote:
> Unrelated formatting changes? (Feel free to land separately)
Probably, I just routinely clang-format the whole file instead of just isolated 
changes.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:67-68
+
+// Undefining a macro invalidates adjacent macros
+// from being considered as an enum.
+#define REMOVED1 1

aaron.ballman wrote:
> What about an #undef that's not adjacent to any macros? e.g.,
> ```
> #define FOO 1
> #define BAR 2
> #define BAZ 3
> 
> int i = 12;
> 
> #if defined(FROBBLE)
> #undef FOO
> #endif
> ```
> I'm worried that perhaps other code elsewhere will be checking `defined(FOO)` 
> perhaps in cases conditionally compiled away, and switching `FOO` to be an 
> enum constant will break other configurations. To be honest, I'm a bit 
> worried about that for all of the transformations here... and I don't know a 
> good way to address that aside from "don't use the check". It'd be 
> interesting to know what kind of false positive rate we have for the fixes if 
> we ran it over a large corpus of code.
Yeah, the problem arises whenever you make any changes to a header file.  Did 
you also change all translation units that include the header?  What about 
conditionally compiled code that was "off" in the translation unit for the 
automated change?  Currently, we don't have a way of analyzing a group of 
translation units together for a cohesive change, nor do we have any way of 
inspecting more deeply into conditionally compiled code.  Addressing those 
concerns is beyond the scope of this check (or any clang-tidy check) as it 
involves improvements to the entire infrastructure.

However, I think it is worth noting in the documentation about possible 
caveats.  I think the way clang

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 3 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:67-68
+
+// Undefining a macro invalidates adjacent macros
+// from being considered as an enum.
+#define REMOVED1 1

LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > What about an #undef that's not adjacent to any macros? e.g.,
> > ```
> > #define FOO 1
> > #define BAR 2
> > #define BAZ 3
> > 
> > int i = 12;
> > 
> > #if defined(FROBBLE)
> > #undef FOO
> > #endif
> > ```
> > I'm worried that perhaps other code elsewhere will be checking 
> > `defined(FOO)` perhaps in cases conditionally compiled away, and switching 
> > `FOO` to be an enum constant will break other configurations. To be honest, 
> > I'm a bit worried about that for all of the transformations here... and I 
> > don't know a good way to address that aside from "don't use the check". 
> > It'd be interesting to know what kind of false positive rate we have for 
> > the fixes if we ran it over a large corpus of code.
> Yeah, the problem arises whenever you make any changes to a header file.  Did 
> you also change all translation units that include the header?  What about 
> conditionally compiled code that was "off" in the translation unit for the 
> automated change?  Currently, we don't have a way of analyzing a group of 
> translation units together for a cohesive change, nor do we have any way of 
> inspecting more deeply into conditionally compiled code.  Addressing those 
> concerns is beyond the scope of this check (or any clang-tidy check) as it 
> involves improvements to the entire infrastructure.
> 
> However, I think it is worth noting in the documentation about possible 
> caveats.  I think the way clang-tidy avoids this problem now is that you have 
> to request fixes and the default mode is to issue warnings and leave it up to 
> the reader as to whether or not they should apply the fixes.
> 
> I believe I already have logic to disqualify any cluster of macros where any 
> one of them are used in a preprocessor condition (that was the last 
> functional change I made to this check).  Looks like I need to extend that 
> slightly to include checking for macros that are `#undef`'ed.
OK, looks like I was already handling this, LOL.  See line 135

```
// Undefining an enum-like macro results in the enum set being dropped.
```


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

https://reviews.llvm.org/D117522

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

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I think I've got all the changes incorporated, but I'm getting a test failure 
so I haven't uploaded a new diff.

Honestly, I don't understand the test failure output.  On Windows, the test 
failure output is all mangled and
difficult to interpret correctly as to what exactly is the problem, so I'm 
still trying to figure it out.

Improving the readability of the test failures is one of the things I would 
like to address in a future change.
I suspect it's only a problem on Windows as it seems most of the clang-tidy 
devs are using unix?


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

https://reviews.llvm.org/D117522

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

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I've worked through this issue before, I just need to spend some time on it.

The basic problem is that the test fails and dumps a bunch of output to "help" 
you
understand the failure, but the way it is formatted and mangled doesn't end up
being useful.


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

https://reviews.llvm.org/D117522

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

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:315
+  if (Info->isFunctionLike() || Info->isBuiltinMacro() ||
+  Info->tokens().empty() || Info->tokens().size() > 2)
+return;

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > We don't seem to have any tests for literal suffixes and I *think* those 
> > > will show up here as additional tokens after the literal. e.g., `#define 
> > > FOO +1ULL`, so I think the size check here may be a problem.
> > On an earlier iteration I had some tests for literals with suffixes and the 
> > suffix is included in the literal token.  I can add some test cases just to 
> > make it clear what the behavior is when they are encountered.
> Thanks, I'd appreciate adding the tests just to be sure we handle the case 
> properly.
I added tests in my latest diff, which I'll upload now even though the tests 
are failing.  (I'll upload again after I've got the test failure figured out.)



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:318
+
+  // It can be +Lit, -Lit or just Lit.
+  Token Tok = Info->tokens().front();

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > It's worth pointing out that both of these are expressions that operate 
> > > on a literal, and if we're going to allow expressions that operator on a 
> > > literal, why only these two? e.g. why not allow `#define FOO ~0U` or 
> > > `#define BAR FOO + 1`? Someday (not today), it would be nice for this to 
> > > work on any expression that's a valid constant expression.
> > A later enhancement can generalize to literal expressions (which are valid 
> > initializers for an enumeration), but I wanted to cover the most common 
> > case of simple negative integers in this first pass.
> I'm less worried about the arbitrary constant expressions than I am about not 
> supporting `~` because `~0U` is not uncommon in macros as a way to set all 
> bits to 1. It's certainly more common than seeing a unary `+`, at least in my 
> experience. However, an even more important use case that I should have 
> thought of first is surrounding the value in parens (which is another common 
> idiom with macros). e.g, `#define ONE (1)`
> 
> Some examples of this in the wild (search the files for `~0U`):
> 
> https://codesearch.isocpp.org/actcd19/main/l/linux/linux_4.15.17-1/drivers/gpu/drm/i915/gvt/handlers.c
> https://codesearch.isocpp.org/actcd19/main/w/wine/wine_4.0-1/dlls/d3d8/d3d8_private.h
> https://codesearch.isocpp.org/actcd19/main/q/qtwebengine-opensource-src/qtwebengine-opensource-src_5.11.3+dfsg-2/src/3rdparty/chromium/third_party/vulkan/include/vulkan/vulkan.h
> 
> (There's plenty of other examples to be found on the same site.)
> 
> I'm fine not completing the set in the initial patch, but the current 
> behavior is a bit confusing (`+` is almost of negligible importance). I think 
> `~` and parens need to be supported; I'd prefer in this patch, but I'm fine 
> if it comes in a subsequent patch so long as those two are supported before 
> we release.
The difficulty in supporting more complex expressions is that we have **NO** 
AST support here and it involves manually matching tokens in the macro 
definition.

However, your point about `~` is well taken and that's easy to add based on 
what I've got here.  I thought it important to handle negative literals, so I 
added support for unary `-`.  I added support for unary `+` out of symmetry.



Comment at: clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp:88-92
+CheckFactories.registerCheck(
+"modernize-use-equals-default");
 CheckFactories.registerCheck(
 "modernize-use-equals-delete");
+CheckFactories.registerCheck("modernize-use-nodiscard");

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > Unrelated formatting changes? (Feel free to land separately)
> > Probably, I just routinely clang-format the whole file instead of just 
> > isolated changes.
> Please don't clang-format the whole file (it makes code reviews more 
> difficult; we document this request a bit in 
> https://llvm.org/docs/CodingStandards.html#introduction), there's a script 
> for formatting just the isolated changes: 
> https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting 
> that I've found works really well in most cases.
Most of the time it doesn't result in any new diffs besides those I'm adding 
myself.  This is particularly true because most of the time I'm contributing 
new checks (which means whole new files) or fixing bugs on my existing checks 
(which have already had the whole file clang-format'ed).

I was unaware of the script, I'll take a look at that, thanks.

Should we cross link to the docs for this script

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 416550.
LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added a comment.

- Update from review comments
- `check-clang-tools` is reporting a test failure that still needs to be 
diagnosed (I think it is a mismatch between a CHECK-MESSAGES line and the exact 
output)


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

https://reviews.llvm.org/D117522

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum2.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum3.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -0,0 +1,228 @@
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum
+// C++14 or later required for binary literals.
+
+#if 1
+#include "modernize-macro-to-enum.h"
+
+// These macros are skipped due to being inside a conditional compilation block.
+#define GOO_RED 1
+#define GOO_GREEN 2
+#define GOO_BLUE 3
+
+#endif
+
+#define RED 0xFF
+#define GREEN 0x00FF00
+#define BLUE 0xFF
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum [modernize-macro-to-enum]
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'RED' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GREEN' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BLUE' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: RED = 0xFF,
+// CHECK-FIXES-NEXT: GREEN = 0x00FF00,
+// CHECK-FIXES-NEXT: BLUE = 0xFF
+// CHECK-FIXES-NEXT: };
+
+// Verify that comments are preserved.
+#define CoordModeOrigin 0   /* relative to the origin */
+#define CoordModePrevious   1   /* relative to previous point */
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModeOrigin' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModePrevious' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: CoordModeOrigin = 0,   /* relative to the origin */
+// CHECK-FIXES-NEXT: CoordModePrevious =   1   /* relative to previous point */
+// CHECK-FIXES-NEXT: };
+
+// Verify that multiline comments are preserved.
+#define BadDrawable 9   /* parameter not a Pixmap or Window */
+#define BadAccess   10  /* depending on context:
+- key/button already grabbed
+- attempt to free an illegal 
+  cmap entry 
+- attempt to store into a read-only 
+  color map entry. */
+// - attempt to modify the access control
+//   list from other than the local host.
+//
+#define BadAlloc11  /* insufficient resources */
+// CHECK-MESSAGES: :[[@LINE-11]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadDrawable' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadAccess' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BadAlloc' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: BadDrawable = 9,   /* parameter not a Pixmap or Window */
+// CHECK-FIXES-NEXT: BadAccess =   10,  /* depending on context:
+// CHECK-FIXES-NEXT: - key/button already grabbed
+// CHECK-FIXES-NEXT: - attempt to free an illegal 
+// CHECK-FIXES-NEXT:   cmap entry 
+// CHECK-FIXES-NEXT: - attempt to store into a read-only 
+// CHECK-FIXES-NEXT:

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 3 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47
+CRLF,
+CRLFCR,
+  };

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > I'm a bit confused by this one as this is not a valid line ending (it's 
> > > either three valid line endings or two valid line endings, depending on 
> > > how you look at it). Can you explain why this is needed?
> > It's a state machine, where the states are named for what we've seen so far 
> > and we're looking for //two// consecutive line endings, not just one.  Does 
> > it make sense now?
> Thanks, I understood it was a state machine, but it's a confused one to me. 
> `\r` was the line ending on Mac Classic, I've not seen it used outside of 
> that platform (and I've not seen anyone write code for that platform in a 
> long time). So, to me, the only valid combinations of line endings to worry 
> about are: `LF LF`; `CRLF CRLF`; `CRLF LF`; `LF CRLF`.
> 
> `LF LF` returns false (Nothing -> LF -> return false)
> `CRLF CRLF` returns false (Nothing -> CR -> CRLF -> CRLFCR -> return false)
> `CRLF LF` returns true (Nothing -> CR -> CRLF -> LF -> finish loop)
> `LF CRLF` returns true (Nothing -> LF -> CR -> CRLF -> finish loop)
> 
> (If you also intend to support Mac Classic line endings for some reason, this 
> gets even more complicated.)
I was trying to follow "be liberal in what you accept as input and conservative 
in what you generate as output" maxim.  I can remove the `CR` as a line ending 
case if you think it's too obscure.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:67-68
+
+// Undefining a macro invalidates adjacent macros
+// from being considered as an enum.
+#define REMOVED1 1

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > LegalizeAdulthood wrote:
> > > aaron.ballman wrote:
> > > > What about an #undef that's not adjacent to any macros? e.g.,
> > > > ```
> > > > #define FOO 1
> > > > #define BAR 2
> > > > #define BAZ 3
> > > > 
> > > > int i = 12;
> > > > 
> > > > #if defined(FROBBLE)
> > > > #undef FOO
> > > > #endif
> > > > ```
> > > > I'm worried that perhaps other code elsewhere will be checking 
> > > > `defined(FOO)` perhaps in cases conditionally compiled away, and 
> > > > switching `FOO` to be an enum constant will break other configurations. 
> > > > To be honest, I'm a bit worried about that for all of the 
> > > > transformations here... and I don't know a good way to address that 
> > > > aside from "don't use the check". It'd be interesting to know what kind 
> > > > of false positive rate we have for the fixes if we ran it over a large 
> > > > corpus of code.
> > > Yeah, the problem arises whenever you make any changes to a header file.  
> > > Did you also change all translation units that include the header?  What 
> > > about conditionally compiled code that was "off" in the translation unit 
> > > for the automated change?  Currently, we don't have a way of analyzing a 
> > > group of translation units together for a cohesive change, nor do we have 
> > > any way of inspecting more deeply into conditionally compiled code.  
> > > Addressing those concerns is beyond the scope of this check (or any 
> > > clang-tidy check) as it involves improvements to the entire 
> > > infrastructure.
> > > 
> > > However, I think it is worth noting in the documentation about possible 
> > > caveats.  I think the way clang-tidy avoids this problem now is that you 
> > > have to request fixes and the default mode is to issue warnings and leave 
> > > it up to the reader as to whether or not they should apply the fixes.
> > > 
> > > I believe I already have logic to disqualify any cluster of macros where 
> > > any one of them are used in a preprocessor condition (that was the last 
> > > functional change I made to this check).  Looks like I need to extend 
> > > that slightly to include checking for macros that are `#undef`'ed.
> > OK, looks like I was already handling this, LOL.  See line 135
> > 
> > ```
> > // Undefining an enum-like macro results in the enum set being dropped.
> > ```
> Yeah, you already have the code for handling this somewhat (that's one of the 
> reasons why I brought this particular use case up). My greater concern is: 
> how many false positives does this check generate on real world code? 
> Documentation may help alleviate those concerns well enough, but if the false 
> positive rate is sufficiently high that you basically have to disable this 
> check for real world code, we need to do better. I don't fully trust my 
> intuition on this one because preprocessor code in the real world has 40+ 
> years worth of accumulated oddities, so having some actual measurements 
> against real world code would be very informative.
In the latest diff, I added a test case to make it cle

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D117522#3392122 , @aaron.ballman 
wrote:

> In D117522#3390136 , 
> @LegalizeAdulthood wrote:
>
>> I think I've got all the changes incorporated, but I'm getting a test 
>> failure so I haven't uploaded a new diff.
>
> If you're able to post the output you're getting, I can try to help 
> psychically debug it.

I finally figured it out!  I previously had written:

  void MacroToEnumCallbacks::warnMacroEnum(const EnumMacro &Macro) const {
Check->diag(Macro.Directive->getLocation(),
"macro '%0' defines an integral constant; prefer an enum 
instead")
<< Macro.Name.getIdentifierInfo()->getName();
  }

and you suggested that I could drop the `->getName()` as it had an insertion 
operator
for identifier info.  Well, that was true, but what I didn't realize is that 
this would add an
extra single quotes around the identifier, so my diagnostic output now had 
doubled-up
single quotes everywhere and I couldn't figure out what was doing this as I 
couldn't find
doubled up single quotes in my format string and I was suspecting the python 
script
was somehow treating single quotes special somewhere.

Once I accounted for this, everything passes, so I'll be uploading a new diff 
shortly.


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

https://reviews.llvm.org/D117522

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

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 416624.
LegalizeAdulthood added a comment.

- Tests pass again


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

https://reviews.llvm.org/D117522

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum2.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum3.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -0,0 +1,228 @@
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum
+// C++14 or later required for binary literals.
+
+#if 1
+#include "modernize-macro-to-enum.h"
+
+// These macros are skipped due to being inside a conditional compilation block.
+#define GOO_RED 1
+#define GOO_GREEN 2
+#define GOO_BLUE 3
+
+#endif
+
+#define RED 0xFF
+#define GREEN 0x00FF00
+#define BLUE 0xFF
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum [modernize-macro-to-enum]
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'RED' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GREEN' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BLUE' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: RED = 0xFF,
+// CHECK-FIXES-NEXT: GREEN = 0x00FF00,
+// CHECK-FIXES-NEXT: BLUE = 0xFF
+// CHECK-FIXES-NEXT: };
+
+// Verify that comments are preserved.
+#define CoordModeOrigin 0   /* relative to the origin */
+#define CoordModePrevious   1   /* relative to previous point */
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModeOrigin' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModePrevious' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: CoordModeOrigin = 0,   /* relative to the origin */
+// CHECK-FIXES-NEXT: CoordModePrevious =   1   /* relative to previous point */
+// CHECK-FIXES-NEXT: };
+
+// Verify that multiline comments are preserved.
+#define BadDrawable 9   /* parameter not a Pixmap or Window */
+#define BadAccess   10  /* depending on context:
+- key/button already grabbed
+- attempt to free an illegal 
+  cmap entry 
+- attempt to store into a read-only 
+  color map entry. */
+// - attempt to modify the access control
+//   list from other than the local host.
+//
+#define BadAlloc11  /* insufficient resources */
+// CHECK-MESSAGES: :[[@LINE-11]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadDrawable' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadAccess' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BadAlloc' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: BadDrawable = 9,   /* parameter not a Pixmap or Window */
+// CHECK-FIXES-NEXT: BadAccess =   10,  /* depending on context:
+// CHECK-FIXES-NEXT: - key/button already grabbed
+// CHECK-FIXES-NEXT: - attempt to free an illegal 
+// CHECK-FIXES-NEXT:   cmap entry 
+// CHECK-FIXES-NEXT: - attempt to store into a read-only 
+// CHECK-FIXES-NEXT:   color map entry. */
+// CHECK-FIXES-NEXT: // - attempt to modify the access control
+// CHECK-FIXES-NEXT: //   list from other than

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 416625.
LegalizeAdulthood added a comment.

- Recognize bitwise negated integers


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

https://reviews.llvm.org/D117522

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum2.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum3.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -0,0 +1,239 @@
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum
+// C++14 or later required for binary literals.
+
+#if 1
+#include "modernize-macro-to-enum.h"
+
+// These macros are skipped due to being inside a conditional compilation block.
+#define GOO_RED 1
+#define GOO_GREEN 2
+#define GOO_BLUE 3
+
+#endif
+
+#define RED 0xFF
+#define GREEN 0x00FF00
+#define BLUE 0xFF
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum [modernize-macro-to-enum]
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'RED' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GREEN' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BLUE' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: RED = 0xFF,
+// CHECK-FIXES-NEXT: GREEN = 0x00FF00,
+// CHECK-FIXES-NEXT: BLUE = 0xFF
+// CHECK-FIXES-NEXT: };
+
+// Verify that comments are preserved.
+#define CoordModeOrigin 0   /* relative to the origin */
+#define CoordModePrevious   1   /* relative to previous point */
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModeOrigin' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModePrevious' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: CoordModeOrigin = 0,   /* relative to the origin */
+// CHECK-FIXES-NEXT: CoordModePrevious =   1   /* relative to previous point */
+// CHECK-FIXES-NEXT: };
+
+// Verify that multiline comments are preserved.
+#define BadDrawable 9   /* parameter not a Pixmap or Window */
+#define BadAccess   10  /* depending on context:
+- key/button already grabbed
+- attempt to free an illegal 
+  cmap entry 
+- attempt to store into a read-only 
+  color map entry. */
+// - attempt to modify the access control
+//   list from other than the local host.
+//
+#define BadAlloc11  /* insufficient resources */
+// CHECK-MESSAGES: :[[@LINE-11]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadDrawable' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadAccess' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BadAlloc' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: BadDrawable = 9,   /* parameter not a Pixmap or Window */
+// CHECK-FIXES-NEXT: BadAccess =   10,  /* depending on context:
+// CHECK-FIXES-NEXT: - key/button already grabbed
+// CHECK-FIXES-NEXT: - attempt to free an illegal 
+// CHECK-FIXES-NEXT:   cmap entry 
+// CHECK-FIXES-NEXT: - attempt to store into a read-only 
+// CHECK-FIXES-NEXT:   color map entry. */
+// CHECK-FIXES-NEXT: // - attempt to modify the access control
+// CHECK-FIXES-NEXT: //   li

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:318
+
+  // It can be +Lit, -Lit or just Lit.
+  Token Tok = Info->tokens().front();

LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > LegalizeAdulthood wrote:
> > > aaron.ballman wrote:
> > > > It's worth pointing out that both of these are expressions that operate 
> > > > on a literal, and if we're going to allow expressions that operator on 
> > > > a literal, why only these two? e.g. why not allow `#define FOO ~0U` or 
> > > > `#define BAR FOO + 1`? Someday (not today), it would be nice for this 
> > > > to work on any expression that's a valid constant expression.
> > > A later enhancement can generalize to literal expressions (which are 
> > > valid initializers for an enumeration), but I wanted to cover the most 
> > > common case of simple negative integers in this first pass.
> > I'm less worried about the arbitrary constant expressions than I am about 
> > not supporting `~` because `~0U` is not uncommon in macros as a way to set 
> > all bits to 1. It's certainly more common than seeing a unary `+`, at least 
> > in my experience. However, an even more important use case that I should 
> > have thought of first is surrounding the value in parens (which is another 
> > common idiom with macros). e.g, `#define ONE (1)`
> > 
> > Some examples of this in the wild (search the files for `~0U`):
> > 
> > https://codesearch.isocpp.org/actcd19/main/l/linux/linux_4.15.17-1/drivers/gpu/drm/i915/gvt/handlers.c
> > https://codesearch.isocpp.org/actcd19/main/w/wine/wine_4.0-1/dlls/d3d8/d3d8_private.h
> > https://codesearch.isocpp.org/actcd19/main/q/qtwebengine-opensource-src/qtwebengine-opensource-src_5.11.3+dfsg-2/src/3rdparty/chromium/third_party/vulkan/include/vulkan/vulkan.h
> > 
> > (There's plenty of other examples to be found on the same site.)
> > 
> > I'm fine not completing the set in the initial patch, but the current 
> > behavior is a bit confusing (`+` is almost of negligible importance). I 
> > think `~` and parens need to be supported; I'd prefer in this patch, but 
> > I'm fine if it comes in a subsequent patch so long as those two are 
> > supported before we release.
> The difficulty in supporting more complex expressions is that we have **NO** 
> AST support here and it involves manually matching tokens in the macro 
> definition.
> 
> However, your point about `~` is well taken and that's easy to add based on 
> what I've got here.  I thought it important to handle negative literals, so I 
> added support for unary `-`.  I added support for unary `+` out of symmetry.
I've added support for bitwise negated integers.  I didn't go further and try 
to recognize parenthesized literals (this just seems dumb, anyway... the extra 
parentheses do nothing and aren't ever needed).


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

https://reviews.llvm.org/D117522

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

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 416627.
LegalizeAdulthood added a comment.

- Undo change introduced by clang-format


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

https://reviews.llvm.org/D117522

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum2.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-macro-to-enum/modernize-macro-to-enum3.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -0,0 +1,239 @@
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum
+// C++14 or later required for binary literals.
+
+#if 1
+#include "modernize-macro-to-enum.h"
+
+// These macros are skipped due to being inside a conditional compilation block.
+#define GOO_RED 1
+#define GOO_GREEN 2
+#define GOO_BLUE 3
+
+#endif
+
+#define RED 0xFF
+#define GREEN 0x00FF00
+#define BLUE 0xFF
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: replace macro with enum [modernize-macro-to-enum]
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'RED' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'GREEN' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BLUE' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: RED = 0xFF,
+// CHECK-FIXES-NEXT: GREEN = 0x00FF00,
+// CHECK-FIXES-NEXT: BLUE = 0xFF
+// CHECK-FIXES-NEXT: };
+
+// Verify that comments are preserved.
+#define CoordModeOrigin 0   /* relative to the origin */
+#define CoordModePrevious   1   /* relative to previous point */
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModeOrigin' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'CoordModePrevious' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: CoordModeOrigin = 0,   /* relative to the origin */
+// CHECK-FIXES-NEXT: CoordModePrevious =   1   /* relative to previous point */
+// CHECK-FIXES-NEXT: };
+
+// Verify that multiline comments are preserved.
+#define BadDrawable 9   /* parameter not a Pixmap or Window */
+#define BadAccess   10  /* depending on context:
+- key/button already grabbed
+- attempt to free an illegal 
+  cmap entry 
+- attempt to store into a read-only 
+  color map entry. */
+// - attempt to modify the access control
+//   list from other than the local host.
+//
+#define BadAlloc11  /* insufficient resources */
+// CHECK-MESSAGES: :[[@LINE-11]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadDrawable' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-12]]:9: warning: macro 'BadAccess' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: macro 'BadAlloc' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: BadDrawable = 9,   /* parameter not a Pixmap or Window */
+// CHECK-FIXES-NEXT: BadAccess =   10,  /* depending on context:
+// CHECK-FIXES-NEXT: - key/button already grabbed
+// CHECK-FIXES-NEXT: - attempt to free an illegal 
+// CHECK-FIXES-NEXT:   cmap entry 
+// CHECK-FIXES-NEXT: - attempt to store into a read-only 
+// CHECK-FIXES-NEXT:   color map entry. */
+// CHECK-FIXES-NEXT: // - attempt to modify the access control
+// CHECK-FIXES-NEXT: // 

[PATCH] D123349: [clang-tidy] Deal with keyword tokens in preprocessor conditions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: aaron.ballman.
LegalizeAdulthood added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
LegalizeAdulthood requested review of this revision.

When a "keyword" token like __restrict was present in a macro condition,
modernize-macro-to-enum would assert in non-release builds.  However,
even for a "keyword" token, calling getIdentifierInfo()->getName() would
retrieve the text of the token, which is what we want.  Our intention is
to scan names that appear in conditional expressions in potential enum
clusters and invalidate those clusters if they contain the name.

Also, guard against "raw identifiers" appearing as potential enums.
This shouldn't happen, but it doesn't hurt to generalize the code.

Fixes #54775


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123349

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -208,6 +208,10 @@
 #define IFNDEF3 3
 #endif
 
+// Sometimes an argument to ifdef can be classified as a keyword token.
+#ifdef __restrict
+#endif
+
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -278,20 +278,17 @@
   }
 }
 
+inline StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()
+ : Tok.getIdentifierInfo()->getName();
+}
+
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  std::string Id;
-  if (MacroNameTok.is(tok::raw_identifier))
-Id = MacroNameTok.getRawIdentifier().str();
-  else if (MacroNameTok.is(tok::identifier))
-Id = MacroNameTok.getIdentifierInfo()->getName().str();
-  else {
-assert(false && "Expected either an identifier or raw identifier token");
-return;
-  }
+  std::string Id = getTokenName(MacroNameTok).str();
 
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
-  return Macro.Name.getIdentifierInfo()->getName().str() == Id;
+  return getTokenName(Macro.Name).str() == Id;
 });
   });
 }
@@ -355,8 +352,7 @@
   const MacroDefinition &MD,
   const MacroDirective *Undef) {
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
-return Macro.Name.getIdentifierInfo()->getName() ==
-   MacroNameTok.getIdentifierInfo()->getName();
+return getTokenName(Macro.Name) == getTokenName(MacroNameTok);
   };
 
   auto It = llvm::find_if(Enums, [MatchesToken](const MacroList &MacroList) {
@@ -432,8 +428,8 @@
 
 void MacroToEnumCallbacks::warnMacroEnum(const EnumMacro &Macro) const {
   Check->diag(Macro.Directive->getLocation(),
-  "macro %0 defines an integral constant; prefer an enum instead")
-  << Macro.Name.getIdentifierInfo();
+  "macro '%0' defines an integral constant; prefer an enum 
instead")
+  << getTokenName(Macro.Name).str();
 }
 
 void MacroToEnumCallbacks::fixEnumMacro(const MacroList &MacroList) const {


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -208,6 +208,10 @@
 #define IFNDEF3 3
 #endif
 
+// Sometimes an argument to ifdef can be classified as a keyword token.
+#ifdef __restrict
+#endif
+
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -278,20 +278,17 @@
   }
 }
 
+inline StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()
+ : Tok.getIdentifierInfo()->getName();
+}
+
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  std::string Id;
-  if (MacroNameTok.is(tok::raw_identifier))
-Id = MacroNameTok

[PATCH] D123349: [clang-tidy] Deal with keyword tokens in preprocessor conditions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 4 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:281
 
+inline StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()

njames93 wrote:
> inline is pretty redundant here. Did you mean to make this static?
ReSharper flagged this as redundant as well, but I'm not sure I understand why 
inline is redundant here.

Static is definitely redundant because this is all inside an anonymous 
namespace block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123349

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

[PATCH] D123349: [clang-tidy] Deal with keyword tokens in preprocessor conditions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 421603.
LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.

Update from review comments


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

https://reviews.llvm.org/D123349

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -208,6 +208,10 @@
 #define IFNDEF3 3
 #endif
 
+// Sometimes an argument to ifdef can be classified as a keyword token.
+#ifdef __restrict
+#endif
+
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -278,20 +278,17 @@
   }
 }
 
+StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()
+ : Tok.getIdentifierInfo()->getName();
+}
+
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  std::string Id;
-  if (MacroNameTok.is(tok::raw_identifier))
-Id = MacroNameTok.getRawIdentifier().str();
-  else if (MacroNameTok.is(tok::identifier))
-Id = MacroNameTok.getIdentifierInfo()->getName().str();
-  else {
-assert(false && "Expected either an identifier or raw identifier token");
-return;
-  }
+  StringRef Id = getTokenName(MacroNameTok);
 
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
-  return Macro.Name.getIdentifierInfo()->getName().str() == Id;
+  return getTokenName(Macro.Name) == Id;
 });
   });
 }
@@ -355,8 +352,7 @@
   const MacroDefinition &MD,
   const MacroDirective *Undef) {
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
-return Macro.Name.getIdentifierInfo()->getName() ==
-   MacroNameTok.getIdentifierInfo()->getName();
+return getTokenName(Macro.Name) == getTokenName(MacroNameTok);
   };
 
   auto It = llvm::find_if(Enums, [MatchesToken](const MacroList &MacroList) {
@@ -432,8 +428,8 @@
 
 void MacroToEnumCallbacks::warnMacroEnum(const EnumMacro &Macro) const {
   Check->diag(Macro.Directive->getLocation(),
-  "macro %0 defines an integral constant; prefer an enum instead")
-  << Macro.Name.getIdentifierInfo();
+  "macro '%0' defines an integral constant; prefer an enum 
instead")
+  << getTokenName(Macro.Name);
 }
 
 void MacroToEnumCallbacks::fixEnumMacro(const MacroList &MacroList) const {


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -208,6 +208,10 @@
 #define IFNDEF3 3
 #endif
 
+// Sometimes an argument to ifdef can be classified as a keyword token.
+#ifdef __restrict
+#endif
+
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -278,20 +278,17 @@
   }
 }
 
+StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()
+ : Tok.getIdentifierInfo()->getName();
+}
+
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  std::string Id;
-  if (MacroNameTok.is(tok::raw_identifier))
-Id = MacroNameTok.getRawIdentifier().str();
-  else if (MacroNameTok.is(tok::identifier))
-Id = MacroNameTok.getIdentifierInfo()->getName().str();
-  else {
-assert(false && "Expected either an identifier or raw identifier token");
-return;
-  }
+  StringRef Id = getTokenName(MacroNameTok);
 
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
-  return Macro.Name.getIdentifierInfo()->getName().str() == Id;
+  return getTokenName(Macro.Name) == Id;
 });
   });
 }
@@ -355,8 +352,7 @@
   const MacroDefinition &MD,
   const

[PATCH] D123349: [clang-tidy] Deal with keyword tokens in preprocessor conditions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:281
 
+inline StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > njames93 wrote:
> > > inline is pretty redundant here. Did you mean to make this static?
> > ReSharper flagged this as redundant as well, but I'm not sure I understand 
> > why inline is redundant here.
> > 
> > Static is definitely redundant because this is all inside an anonymous 
> > namespace block.
> > ReSharper flagged this as redundant as well, but I'm not sure I understand 
> > why inline is redundant here.
> 
> `inline` has no real semantic effect there -- it's an internal function so 
> the optimizer is free to inline it at its whim if it wants.
> 
> > Static is definitely redundant because this is all inside an anonymous 
> > namespace block.
> 
> We should correct that in a follow-up NFC commit, that's something we 
> recommend against in the style guide 
> (https://llvm.org/docs/CodingStandards.html#anonymous-namespaces) for exactly 
> this scenario (it's not immediately clear when reading the declaration that 
> it already had internal linkage).
I don't mind fixing it now since it's a new function


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

https://reviews.llvm.org/D123349

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

[PATCH] D123349: [clang-tidy] Deal with keyword tokens in preprocessor conditions

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 421628.
LegalizeAdulthood added a comment.

Update from review comments


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

https://reviews.llvm.org/D123349

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -208,6 +208,10 @@
 #define IFNDEF3 3
 #endif
 
+// Sometimes an argument to ifdef can be classified as a keyword token.
+#ifdef __restrict
+#endif
+
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -92,6 +92,11 @@
   Begin, End, [](char C) { return C == '.' || std::toupper(C) == 'E'; });
 }
 
+static StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()
+ : Tok.getIdentifierInfo()->getName();
+}
+
 namespace {
 
 struct EnumMacro {
@@ -279,19 +284,11 @@
 }
 
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  std::string Id;
-  if (MacroNameTok.is(tok::raw_identifier))
-Id = MacroNameTok.getRawIdentifier().str();
-  else if (MacroNameTok.is(tok::identifier))
-Id = MacroNameTok.getIdentifierInfo()->getName().str();
-  else {
-assert(false && "Expected either an identifier or raw identifier token");
-return;
-  }
+  StringRef Id = getTokenName(MacroNameTok);
 
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
-  return Macro.Name.getIdentifierInfo()->getName().str() == Id;
+  return getTokenName(Macro.Name) == Id;
 });
   });
 }
@@ -355,8 +352,7 @@
   const MacroDefinition &MD,
   const MacroDirective *Undef) {
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
-return Macro.Name.getIdentifierInfo()->getName() ==
-   MacroNameTok.getIdentifierInfo()->getName();
+return getTokenName(Macro.Name) == getTokenName(MacroNameTok);
   };
 
   auto It = llvm::find_if(Enums, [MatchesToken](const MacroList &MacroList) {
@@ -432,8 +428,8 @@
 
 void MacroToEnumCallbacks::warnMacroEnum(const EnumMacro &Macro) const {
   Check->diag(Macro.Directive->getLocation(),
-  "macro %0 defines an integral constant; prefer an enum instead")
-  << Macro.Name.getIdentifierInfo();
+  "macro '%0' defines an integral constant; prefer an enum 
instead")
+  << getTokenName(Macro.Name);
 }
 
 void MacroToEnumCallbacks::fixEnumMacro(const MacroList &MacroList) const {


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -208,6 +208,10 @@
 #define IFNDEF3 3
 #endif
 
+// Sometimes an argument to ifdef can be classified as a keyword token.
+#ifdef __restrict
+#endif
+
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -92,6 +92,11 @@
   Begin, End, [](char C) { return C == '.' || std::toupper(C) == 'E'; });
 }
 
+static StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()
+ : Tok.getIdentifierInfo()->getName();
+}
+
 namespace {
 
 struct EnumMacro {
@@ -279,19 +284,11 @@
 }
 
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  std::string Id;
-  if (MacroNameTok.is(tok::raw_identifier))
-Id = MacroNameTok.getRawIdentifier().str();
-  else if (MacroNameTok.is(tok::identifier))
-Id = MacroNameTok.getIdentifierInfo()->getName().str();
-  else {
-assert(false && "Expected either an identifier or raw identifier token");
-return;
-  }
+  StringRef Id = getTokenName(MacroNameTok);
 
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
-  return Macro.Name.getIdentifierInfo()

[PATCH] D123349: [clang-tidy] Deal with keyword tokens in preprocessor conditions

[Richard via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG88a7508b1fd8: [clang-tidy] Deal with keyword tokens in 
preprocessor conditions (authored by LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123349

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -208,6 +208,10 @@
 #define IFNDEF3 3
 #endif
 
+// Sometimes an argument to ifdef can be classified as a keyword token.
+#ifdef __restrict
+#endif
+
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -92,6 +92,11 @@
   Begin, End, [](char C) { return C == '.' || std::toupper(C) == 'E'; });
 }
 
+static StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()
+ : Tok.getIdentifierInfo()->getName();
+}
+
 namespace {
 
 struct EnumMacro {
@@ -279,19 +284,11 @@
 }
 
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  std::string Id;
-  if (MacroNameTok.is(tok::raw_identifier))
-Id = MacroNameTok.getRawIdentifier().str();
-  else if (MacroNameTok.is(tok::identifier))
-Id = MacroNameTok.getIdentifierInfo()->getName().str();
-  else {
-assert(false && "Expected either an identifier or raw identifier token");
-return;
-  }
+  StringRef Id = getTokenName(MacroNameTok);
 
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
-  return Macro.Name.getIdentifierInfo()->getName().str() == Id;
+  return getTokenName(Macro.Name) == Id;
 });
   });
 }
@@ -355,8 +352,7 @@
   const MacroDefinition &MD,
   const MacroDirective *Undef) {
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
-return Macro.Name.getIdentifierInfo()->getName() ==
-   MacroNameTok.getIdentifierInfo()->getName();
+return getTokenName(Macro.Name) == getTokenName(MacroNameTok);
   };
 
   auto It = llvm::find_if(Enums, [MatchesToken](const MacroList &MacroList) {
@@ -432,8 +428,8 @@
 
 void MacroToEnumCallbacks::warnMacroEnum(const EnumMacro &Macro) const {
   Check->diag(Macro.Directive->getLocation(),
-  "macro %0 defines an integral constant; prefer an enum instead")
-  << Macro.Name.getIdentifierInfo();
+  "macro '%0' defines an integral constant; prefer an enum 
instead")
+  << getTokenName(Macro.Name);
 }
 
 void MacroToEnumCallbacks::fixEnumMacro(const MacroList &MacroList) const {


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -208,6 +208,10 @@
 #define IFNDEF3 3
 #endif
 
+// Sometimes an argument to ifdef can be classified as a keyword token.
+#ifdef __restrict
+#endif
+
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -92,6 +92,11 @@
   Begin, End, [](char C) { return C == '.' || std::toupper(C) == 'E'; });
 }
 
+static StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()
+ : Tok.getIdentifierInfo()->getName();
+}
+
 namespace {
 
 struct EnumMacro {
@@ -279,19 +284,11 @@
 }
 
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  std::string Id;
-  if (MacroNameTok.is(tok::raw_identifier))
-Id = MacroNameTok.getRawIdentifier().str();
-  else if (MacroNameTok.is(tok::identifier))
-Id = MacroNameTok.getIdentifierInfo()->getName().str();
-  else {
-assert(false && "Expected either an identifier or raw identifier token");
-return;
-  }
+  StringRef Id = getTokenName(MacroNameTok);
 
   llvm::erase_if(Enums, [&Id](const MacroList &M

[PATCH] D123479: [clang-tidy] Support parenthesized literals in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: aaron.ballman.
LegalizeAdulthood added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
LegalizeAdulthood requested review of this revision.

When scanning a macro expansion to examine it as a candidate enum,
first strip off arbitrary matching parentheses from the outside in,
then examine what remains to see if it is Lit, +Lit, -Lit or ~Lit.
If not, reject it as a possible enum candidate.

Fixes #54843


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123479

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -137,6 +137,41 @@
 // CHECK-FIXES-NEXT: SUFFIX5 = +1ULL
 // CHECK-FIXES-NEXT: };
 
+// A limited form of constant expression is recognized: a parenthesized
+// literal or a parenthesized literal with the unary operators +, - or ~.
+#define PAREN1 (-1)
+#define PAREN2 (1)
+#define PAREN3 (+1)
+#define PAREN4 (~1)
+// CHECK-MESSAGES: :[[@LINE-4]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN1' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN2' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN3' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN4' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: PAREN1 = (-1),
+// CHECK-FIXES-NEXT: PAREN2 = (1),
+// CHECK-FIXES-NEXT: PAREN3 = (+1),
+// CHECK-FIXES-NEXT: PAREN4 = (~1)
+// CHECK-FIXES-NEXT: };
+
+// More complicated parenthesized expressions are excluded.
+// Expansions that are not surrounded by parentheses are excluded.
+// Nested matching parentheses are stripped.
+#define COMPLEX_PAREN1 (x+1)
+#define COMPLEX_PAREN2 (x+1
+#define COMPLEX_PAREN3 (())
+#define COMPLEX_PAREN4 ()
+#define COMPLEX_PAREN5 (+1)
+#define COMPLEX_PAREN6 ((+1))
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN5' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN6' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: COMPLEX_PAREN5 = (+1),
+// CHECK-FIXES-NEXT: COMPLEX_PAREN6 = ((+1))
+// CHECK-FIXES-NEXT: };
+
 // Macros appearing in conditional expressions can't be replaced
 // by enums.
 #define USE_FOO 1
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
@@ -14,10 +14,13 @@
 
 Potential macros for replacement must meet the following constraints:
 
-- Macros must expand only to integral literal tokens.  The unary operators
-  plus, minus and tilde are recognized to allow for positive, negative
-  and bitwise negated integers.  More complicated integral constant
-  expressions are not currently recognized by this check.
+- Macros must expand only to integral literal tokens or simple expressions
+  of literal tokens.  The unary operators plus, minus and tilde are
+  recognized to allow for positive, negative and bitwise negated integers.
+  The above expressions may also be surrounded by a single matching pair of
+  parentheses.
+  More complicated integral constant expressions are not currently recognized
+  by this check.
 - Macros must be defined on sequential source file lines, or with
   only comment lines in between macro definitions.
 - Macros must all be defined in the same source file.
@@ -43,6 +46,7 @@
   #define GREEN 0x00FF00
   #define BLUE  0xFF
 
+  #define TM_NONE (-1) // No method selected.
   #define TM_ONE 1// Use tailored method one.
   #define TM_TWO 2// Use tailored method two.  Method two
   // is preferable to method one.
@@ -59,6 +63,7 @@
   };
 
   enum {
+  TM_NONE = (-1), // No method selected.
   TM_ONE = 1,// Use tailored method one.
   TM_TWO = 2,// Use tailored method two.  Method two
   // is preferable to method one.
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clan

[PATCH] D123479: [clang-tidy] Support parenthesized literals in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D123479#3442062 , @njames93 wrote:

> Would it not be better if these parens were stripped in the fixit as they are 
> unnecessary in the enum value decl?

I prefer checks to do one thing only in a straightforward manner.

It's easier to place things before and after the macro expansion rather than 
modify the contents
of the expansion.

It doesn't feel like the responsibility of this check is to decide which 
parentheses are necessary or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123479

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

[PATCH] D123479: [clang-tidy] Support parenthesized literals in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added a comment.

In D123479#3443401 , @aaron.ballman 
wrote:

> In D123479#3442968 , 
> @LegalizeAdulthood wrote:
>
>> In D123479#3442062 , @njames93 
>> wrote:
>>
>>> Would it not be better if these parens were stripped in the fixit as they 
>>> are unnecessary in the enum value decl?
>
> To me, that would be ideal, but not strictly necessary (and perhaps doesn't 
> scale super well across all fix-its) because the code is still correct with 
> the parens, just a bit ugly. We have other similar situations with fix-its 
> (like formatting).
>
>> I prefer checks to do one thing only in a straightforward manner.
>>
>> It's easier to place things before and after the macro expansion rather than 
>> modify the contents
>> of the expansion.
>>
>> It doesn't feel like the responsibility of this check is to decide which 
>> parentheses are necessary or not.
>
> Hmm, I'm more on the fence.

Me too.  I am just not a fan of an automated check doing "other things" to my 
code
that aren't advertised in the description.  This is a macro-to-enum check not a 
redundant-parens
check.

BTW, many IDEs highlight unnecessary parentheses and have "quick fixes" to 
remove them,
ReSharper for C++ is one such IDE add-on that does this.

I'd prefer there to be a readability-redundant-parentheses check that removes 
unnecessary
parentheses across the board everywhere, rather than it being a weird 
side-effect of turning
my macros into enums.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123479

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

[PATCH] D123479: [clang-tidy] Support parenthesized literals in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 421990.
LegalizeAdulthood added a comment.

Update from review comments


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

https://reviews.llvm.org/D123479

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -137,6 +137,41 @@
 // CHECK-FIXES-NEXT: SUFFIX5 = +1ULL
 // CHECK-FIXES-NEXT: };
 
+// A limited form of constant expression is recognized: a parenthesized
+// literal or a parenthesized literal with the unary operators +, - or ~.
+#define PAREN1 (-1)
+#define PAREN2 (1)
+#define PAREN3 (+1)
+#define PAREN4 (~1)
+// CHECK-MESSAGES: :[[@LINE-4]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN1' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN2' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN3' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN4' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: PAREN1 = (-1),
+// CHECK-FIXES-NEXT: PAREN2 = (1),
+// CHECK-FIXES-NEXT: PAREN3 = (+1),
+// CHECK-FIXES-NEXT: PAREN4 = (~1)
+// CHECK-FIXES-NEXT: };
+
+// More complicated parenthesized expressions are excluded.
+// Expansions that are not surrounded by parentheses are excluded.
+// Nested matching parentheses are stripped.
+#define COMPLEX_PAREN1 (x+1)
+#define COMPLEX_PAREN2 (x+1
+#define COMPLEX_PAREN3 (())
+#define COMPLEX_PAREN4 ()
+#define COMPLEX_PAREN5 (+1)
+#define COMPLEX_PAREN6 ((+1))
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN5' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN6' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: COMPLEX_PAREN5 = (+1),
+// CHECK-FIXES-NEXT: COMPLEX_PAREN6 = ((+1))
+// CHECK-FIXES-NEXT: };
+
 // Macros appearing in conditional expressions can't be replaced
 // by enums.
 #define USE_FOO 1
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
@@ -14,10 +14,12 @@
 
 Potential macros for replacement must meet the following constraints:
 
-- Macros must expand only to integral literal tokens.  The unary operators
-  plus, minus and tilde are recognized to allow for positive, negative
-  and bitwise negated integers.  More complicated integral constant
-  expressions are not currently recognized by this check.
+- Macros must expand only to integral literal tokens or simple expressions
+  of literal tokens.  The unary operators plus, minus and tilde are
+  recognized to allow for positive, negative and bitwise negated integers.
+  The above expressions may also be surrounded by a single matching pair of
+  parentheses.  More complicated integral constant expressions are not
+  recognized by this check.
 - Macros must be defined on sequential source file lines, or with
   only comment lines in between macro definitions.
 - Macros must all be defined in the same source file.
@@ -43,6 +45,7 @@
   #define GREEN 0x00FF00
   #define BLUE  0xFF
 
+  #define TM_NONE (-1) // No method selected.
   #define TM_ONE 1// Use tailored method one.
   #define TM_TWO 2// Use tailored method two.  Method two
   // is preferable to method one.
@@ -59,6 +62,7 @@
   };
 
   enum {
+  TM_NONE = (-1), // No method selected.
   TM_ONE = 1,// Use tailored method one.
   TM_TWO = 2,// Use tailored method two.  Method two
   // is preferable to method one.
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -324,18 +324,51 @@
 return;
 
   const MacroInfo *Info = MD->getMacroInfo();
-  if (Info->isFunctionLike() || Info->isBuiltinMacro() ||
-  Info->tokens().empty() || Info->tokens().size() > 2)
+  ArrayRef MacroTokens = Info->tokens();
+  if (Info->isFunctionLike() || Info->isBuiltinMacro() || Mac

[PATCH] D123479: [clang-tidy] Support parenthesized literals in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 421991.
LegalizeAdulthood added a comment.

- Update documentation to reflect multiple pairs of matching parens


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

https://reviews.llvm.org/D123479

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -137,6 +137,41 @@
 // CHECK-FIXES-NEXT: SUFFIX5 = +1ULL
 // CHECK-FIXES-NEXT: };
 
+// A limited form of constant expression is recognized: a parenthesized
+// literal or a parenthesized literal with the unary operators +, - or ~.
+#define PAREN1 (-1)
+#define PAREN2 (1)
+#define PAREN3 (+1)
+#define PAREN4 (~1)
+// CHECK-MESSAGES: :[[@LINE-4]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN1' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN2' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN3' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN4' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: PAREN1 = (-1),
+// CHECK-FIXES-NEXT: PAREN2 = (1),
+// CHECK-FIXES-NEXT: PAREN3 = (+1),
+// CHECK-FIXES-NEXT: PAREN4 = (~1)
+// CHECK-FIXES-NEXT: };
+
+// More complicated parenthesized expressions are excluded.
+// Expansions that are not surrounded by parentheses are excluded.
+// Nested matching parentheses are stripped.
+#define COMPLEX_PAREN1 (x+1)
+#define COMPLEX_PAREN2 (x+1
+#define COMPLEX_PAREN3 (())
+#define COMPLEX_PAREN4 ()
+#define COMPLEX_PAREN5 (+1)
+#define COMPLEX_PAREN6 ((+1))
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN5' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN6' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: COMPLEX_PAREN5 = (+1),
+// CHECK-FIXES-NEXT: COMPLEX_PAREN6 = ((+1))
+// CHECK-FIXES-NEXT: };
+
 // Macros appearing in conditional expressions can't be replaced
 // by enums.
 #define USE_FOO 1
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
@@ -14,10 +14,12 @@
 
 Potential macros for replacement must meet the following constraints:
 
-- Macros must expand only to integral literal tokens.  The unary operators
-  plus, minus and tilde are recognized to allow for positive, negative
-  and bitwise negated integers.  More complicated integral constant
-  expressions are not currently recognized by this check.
+- Macros must expand only to integral literal tokens or simple expressions
+  of literal tokens.  The unary operators plus, minus and tilde are
+  recognized to allow for positive, negative and bitwise negated integers.
+  The above expressions may also be surrounded by matching pairs of
+  parentheses.  More complicated integral constant expressions are not
+  recognized by this check.
 - Macros must be defined on sequential source file lines, or with
   only comment lines in between macro definitions.
 - Macros must all be defined in the same source file.
@@ -43,6 +45,7 @@
   #define GREEN 0x00FF00
   #define BLUE  0xFF
 
+  #define TM_NONE (-1) // No method selected.
   #define TM_ONE 1// Use tailored method one.
   #define TM_TWO 2// Use tailored method two.  Method two
   // is preferable to method one.
@@ -59,6 +62,7 @@
   };
 
   enum {
+  TM_NONE = (-1), // No method selected.
   TM_ONE = 1,// Use tailored method one.
   TM_TWO = 2,// Use tailored method two.  Method two
   // is preferable to method one.
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -324,18 +324,51 @@
 return;
 
   const MacroInfo *Info = MD->getMacroInfo();
-  if (Info->isFunctionLike() || Info->isBuiltinMacro() ||
-  Info->tokens().empty() || Info->tokens().size() > 2)
+  ArrayRef MacroTokens = Info->tokens();
+  if (Info->isFunctionLike() 

[PATCH] D123479: [clang-tidy] Support parenthesized literals in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd563c2d0e52a: [clang-tidy] Support parenthesized literals in 
modernize-macro-to-enum (authored by LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123479

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -137,6 +137,41 @@
 // CHECK-FIXES-NEXT: SUFFIX5 = +1ULL
 // CHECK-FIXES-NEXT: };
 
+// A limited form of constant expression is recognized: a parenthesized
+// literal or a parenthesized literal with the unary operators +, - or ~.
+#define PAREN1 (-1)
+#define PAREN2 (1)
+#define PAREN3 (+1)
+#define PAREN4 (~1)
+// CHECK-MESSAGES: :[[@LINE-4]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN1' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN2' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN3' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN4' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: PAREN1 = (-1),
+// CHECK-FIXES-NEXT: PAREN2 = (1),
+// CHECK-FIXES-NEXT: PAREN3 = (+1),
+// CHECK-FIXES-NEXT: PAREN4 = (~1)
+// CHECK-FIXES-NEXT: };
+
+// More complicated parenthesized expressions are excluded.
+// Expansions that are not surrounded by parentheses are excluded.
+// Nested matching parentheses are stripped.
+#define COMPLEX_PAREN1 (x+1)
+#define COMPLEX_PAREN2 (x+1
+#define COMPLEX_PAREN3 (())
+#define COMPLEX_PAREN4 ()
+#define COMPLEX_PAREN5 (+1)
+#define COMPLEX_PAREN6 ((+1))
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN5' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN6' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: COMPLEX_PAREN5 = (+1),
+// CHECK-FIXES-NEXT: COMPLEX_PAREN6 = ((+1))
+// CHECK-FIXES-NEXT: };
+
 // Macros appearing in conditional expressions can't be replaced
 // by enums.
 #define USE_FOO 1
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst
@@ -14,10 +14,12 @@
 
 Potential macros for replacement must meet the following constraints:
 
-- Macros must expand only to integral literal tokens.  The unary operators
-  plus, minus and tilde are recognized to allow for positive, negative
-  and bitwise negated integers.  More complicated integral constant
-  expressions are not currently recognized by this check.
+- Macros must expand only to integral literal tokens or simple expressions
+  of literal tokens.  The unary operators plus, minus and tilde are
+  recognized to allow for positive, negative and bitwise negated integers.
+  The above expressions may also be surrounded by matching pairs of
+  parentheses.  More complicated integral constant expressions are not
+  recognized by this check.
 - Macros must be defined on sequential source file lines, or with
   only comment lines in between macro definitions.
 - Macros must all be defined in the same source file.
@@ -43,6 +45,7 @@
   #define GREEN 0x00FF00
   #define BLUE  0xFF
 
+  #define TM_NONE (-1) // No method selected.
   #define TM_ONE 1// Use tailored method one.
   #define TM_TWO 2// Use tailored method two.  Method two
   // is preferable to method one.
@@ -59,6 +62,7 @@
   };
 
   enum {
+  TM_NONE = (-1), // No method selected.
   TM_ONE = 1,// Use tailored method one.
   TM_TWO = 2,// Use tailored method two.  Method two
   // is preferable to method one.
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -324,18 +324,51 @@
 return;
 
   const MacroInfo *Info = MD->getMacroInfo();
-  if (Info->isFunctionLike() || Info->isBuil

[PATCH] D123889: [clang-tidy] Improve macro handling in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added reviewers: aaron.ballman, klimek.
LegalizeAdulthood added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
LegalizeAdulthood requested review of this revision.

When a macro is undef'ed or used in a preprocessor conditional
expression, we need to remember that macro should it later be
defined in the file to an integral value.  We need to exclude
such macro names from being turned into an enum.

Maintain a blacklist of identifiers that we've seen in an
undef or conditional preprocessor directive.  When the file is
done processing, remove all the blacklisted identifiers from
conversion to an enum.

Fixes #54842


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123889

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -181,6 +181,12 @@
 #define USE_IFDEF 1
 #define USE_IFNDEF 1
 
+// Undef'ing first and then defining later should still exclude this macro
+#undef USE_UINT64
+#define USE_UINT64 0
+#undef USE_INT64
+#define USE_INT64 0
+
 #if defined(USE_FOO) && USE_FOO
 extern void foo();
 #else
@@ -243,6 +249,19 @@
 #define IFNDEF3 3
 #endif
 
+// Macros used in conditions are invalidated, even if they look
+// like enums after they are used in conditions.
+#if DEFINED_LATER1
+#endif
+#ifdef DEFINED_LATER2
+#endif
+#ifndef DEFINED_LATER3
+#endif
+
+#define DEFINED_LATER1 1
+#define DEFINED_LATER2 2
+#define DEFINED_LATER3 3
+
 // Sometimes an argument to ifdef can be classified as a keyword token.
 #ifdef __restrict
 #endif
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -193,6 +193,7 @@
   void Endif(SourceLocation Loc, SourceLocation IfLoc) override;
   void PragmaDirective(SourceLocation Loc,
PragmaIntroducerKind Introducer) override;
+  void invalidateExpressionNames();
 
   // After we've seen everything, issue warnings and fix-its.
   void EndOfMainFile() override;
@@ -222,6 +223,7 @@
   void conditionStart(const SourceLocation &Loc);
   void checkCondition(SourceRange ConditionRange);
   void checkName(const Token &MacroNameTok);
+  void rememberExpressionToken(const Token &MacroNameTok);
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
@@ -230,6 +232,7 @@
   const SourceManager &SM;
   SmallVector Enums;
   SmallVector Files;
+  std::vector ExpressionNames;
   FileState *CurrentFile = nullptr;
 };
 
@@ -284,8 +287,9 @@
 }
 
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  StringRef Id = getTokenName(MacroNameTok);
+  rememberExpressionToken(MacroNameTok);
 
+  StringRef Id = getTokenName(MacroNameTok);
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
   return getTokenName(Macro.Name) == Id;
@@ -293,6 +297,14 @@
   });
 }
 
+void MacroToEnumCallbacks::rememberExpressionToken(const Token &MacroNameTok) {
+  std::string Id = getTokenName(MacroNameTok).str();
+  auto Pos = llvm::lower_bound(ExpressionNames, Id);
+  if (Pos == ExpressionNames.end() || *Pos != Id) {
+ExpressionNames.insert(Pos, Id);
+  }
+}
+
 void MacroToEnumCallbacks::FileChanged(SourceLocation Loc,
FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -384,6 +396,8 @@
 void MacroToEnumCallbacks::MacroUndefined(const Token &MacroNameTok,
   const MacroDefinition &MD,
   const MacroDirective *Undef) {
+  rememberExpressionToken(MacroNameTok);
+
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
 return getTokenName(Macro.Name) == getTokenName(MacroNameTok);
   };
@@ -447,7 +461,19 @@
 CurrentFile->GuardScanner = IncludeGuard::IfGuard;
 }
 
+void MacroToEnumCallbacks::invalidateExpressionNames() {
+  for (const std::string &Id : ExpressionNames) {
+llvm::erase_if(Enums, [Id](const MacroList &MacroList) {
+  return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
+return getTokenName(Macro.Name) == Id;
+  });
+});
+  }
+}
+
 void MacroToEnumCallbacks::EndOfMainFile() {
+  invalidateExpressionNames();
+
   for (const MacroList &MacroList : Enums) {
 if (MacroList.empty())
   continue;

[PATCH] D123889: [clang-tidy] Improve macro handling in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

This is an initial pass.  While I believe the code handles other scenarios, I 
still need to add some more test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123889

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

[PATCH] D123889: [clang-tidy] Improve macro handling in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 423258.
LegalizeAdulthood added a comment.

Add more test cases


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

https://reviews.llvm.org/D123889

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -181,6 +181,12 @@
 #define USE_IFDEF 1
 #define USE_IFNDEF 1
 
+// Undef'ing first and then defining later should still exclude this macro
+#undef USE_UINT64
+#define USE_UINT64 0
+#undef USE_INT64
+#define USE_INT64 0
+
 #if defined(USE_FOO) && USE_FOO
 extern void foo();
 #else
@@ -243,6 +249,27 @@
 #define IFNDEF3 3
 #endif
 
+// Macros used in conditions are invalidated, even if they look
+// like enums after they are used in conditions.
+#if DEFINED_LATER1
+#endif
+#ifdef DEFINED_LATER2
+#endif
+#ifndef DEFINED_LATER3
+#endif
+#undef DEFINED_LATER4
+#if ((defined(DEFINED_LATER5) || DEFINED_LATER6) && DEFINED_LATER7) || (DEFINED_LATER8 > 10)
+#endif
+
+#define DEFINED_LATER1 1
+#define DEFINED_LATER2 2
+#define DEFINED_LATER3 3
+#define DEFINED_LATER4 4
+#define DEFINED_LATER5 5
+#define DEFINED_LATER6 6
+#define DEFINED_LATER7 7
+#define DEFINED_LATER8 8
+
 // Sometimes an argument to ifdef can be classified as a keyword token.
 #ifdef __restrict
 #endif
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -193,6 +193,7 @@
   void Endif(SourceLocation Loc, SourceLocation IfLoc) override;
   void PragmaDirective(SourceLocation Loc,
PragmaIntroducerKind Introducer) override;
+  void invalidateExpressionNames();
 
   // After we've seen everything, issue warnings and fix-its.
   void EndOfMainFile() override;
@@ -222,6 +223,7 @@
   void conditionStart(const SourceLocation &Loc);
   void checkCondition(SourceRange ConditionRange);
   void checkName(const Token &MacroNameTok);
+  void rememberExpressionToken(const Token &MacroNameTok);
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
@@ -230,6 +232,7 @@
   const SourceManager &SM;
   SmallVector Enums;
   SmallVector Files;
+  std::vector ExpressionNames;
   FileState *CurrentFile = nullptr;
 };
 
@@ -284,8 +287,9 @@
 }
 
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  StringRef Id = getTokenName(MacroNameTok);
+  rememberExpressionToken(MacroNameTok);
 
+  StringRef Id = getTokenName(MacroNameTok);
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
   return getTokenName(Macro.Name) == Id;
@@ -293,6 +297,14 @@
   });
 }
 
+void MacroToEnumCallbacks::rememberExpressionToken(const Token &MacroNameTok) {
+  std::string Id = getTokenName(MacroNameTok).str();
+  auto Pos = llvm::lower_bound(ExpressionNames, Id);
+  if (Pos == ExpressionNames.end() || *Pos != Id) {
+ExpressionNames.insert(Pos, Id);
+  }
+}
+
 void MacroToEnumCallbacks::FileChanged(SourceLocation Loc,
FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -384,6 +396,8 @@
 void MacroToEnumCallbacks::MacroUndefined(const Token &MacroNameTok,
   const MacroDefinition &MD,
   const MacroDirective *Undef) {
+  rememberExpressionToken(MacroNameTok);
+
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
 return getTokenName(Macro.Name) == getTokenName(MacroNameTok);
   };
@@ -447,7 +461,19 @@
 CurrentFile->GuardScanner = IncludeGuard::IfGuard;
 }
 
+void MacroToEnumCallbacks::invalidateExpressionNames() {
+  for (const std::string &Id : ExpressionNames) {
+llvm::erase_if(Enums, [Id](const MacroList &MacroList) {
+  return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
+return getTokenName(Macro.Name) == Id;
+  });
+});
+  }
+}
+
 void MacroToEnumCallbacks::EndOfMainFile() {
+  invalidateExpressionNames();
+
   for (const MacroList &MacroList : Enums) {
 if (MacroList.empty())
   continue;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123889: [clang-tidy] Improve macro handling in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 423259.
LegalizeAdulthood added a comment.

Make method names symmetric


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

https://reviews.llvm.org/D123889

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -181,6 +181,12 @@
 #define USE_IFDEF 1
 #define USE_IFNDEF 1
 
+// Undef'ing first and then defining later should still exclude this macro
+#undef USE_UINT64
+#define USE_UINT64 0
+#undef USE_INT64
+#define USE_INT64 0
+
 #if defined(USE_FOO) && USE_FOO
 extern void foo();
 #else
@@ -243,6 +249,27 @@
 #define IFNDEF3 3
 #endif
 
+// Macros used in conditions are invalidated, even if they look
+// like enums after they are used in conditions.
+#if DEFINED_LATER1
+#endif
+#ifdef DEFINED_LATER2
+#endif
+#ifndef DEFINED_LATER3
+#endif
+#undef DEFINED_LATER4
+#if ((defined(DEFINED_LATER5) || DEFINED_LATER6) && DEFINED_LATER7) || (DEFINED_LATER8 > 10)
+#endif
+
+#define DEFINED_LATER1 1
+#define DEFINED_LATER2 2
+#define DEFINED_LATER3 3
+#define DEFINED_LATER4 4
+#define DEFINED_LATER5 5
+#define DEFINED_LATER6 6
+#define DEFINED_LATER7 7
+#define DEFINED_LATER8 8
+
 // Sometimes an argument to ifdef can be classified as a keyword token.
 #ifdef __restrict
 #endif
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -193,6 +193,7 @@
   void Endif(SourceLocation Loc, SourceLocation IfLoc) override;
   void PragmaDirective(SourceLocation Loc,
PragmaIntroducerKind Introducer) override;
+  void invalidateExpressionNames();
 
   // After we've seen everything, issue warnings and fix-its.
   void EndOfMainFile() override;
@@ -222,6 +223,7 @@
   void conditionStart(const SourceLocation &Loc);
   void checkCondition(SourceRange ConditionRange);
   void checkName(const Token &MacroNameTok);
+  void rememberExpressionName(const Token &MacroNameTok);
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
@@ -230,6 +232,7 @@
   const SourceManager &SM;
   SmallVector Enums;
   SmallVector Files;
+  std::vector ExpressionNames;
   FileState *CurrentFile = nullptr;
 };
 
@@ -284,8 +287,9 @@
 }
 
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  StringRef Id = getTokenName(MacroNameTok);
+  rememberExpressionName(MacroNameTok);
 
+  StringRef Id = getTokenName(MacroNameTok);
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
   return getTokenName(Macro.Name) == Id;
@@ -293,6 +297,14 @@
   });
 }
 
+void MacroToEnumCallbacks::rememberExpressionName(const Token &MacroNameTok) {
+  std::string Id = getTokenName(MacroNameTok).str();
+  auto Pos = llvm::lower_bound(ExpressionNames, Id);
+  if (Pos == ExpressionNames.end() || *Pos != Id) {
+ExpressionNames.insert(Pos, Id);
+  }
+}
+
 void MacroToEnumCallbacks::FileChanged(SourceLocation Loc,
FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -384,6 +396,8 @@
 void MacroToEnumCallbacks::MacroUndefined(const Token &MacroNameTok,
   const MacroDefinition &MD,
   const MacroDirective *Undef) {
+  rememberExpressionName(MacroNameTok);
+
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
 return getTokenName(Macro.Name) == getTokenName(MacroNameTok);
   };
@@ -447,7 +461,19 @@
 CurrentFile->GuardScanner = IncludeGuard::IfGuard;
 }
 
+void MacroToEnumCallbacks::invalidateExpressionNames() {
+  for (const std::string &Id : ExpressionNames) {
+llvm::erase_if(Enums, [Id](const MacroList &MacroList) {
+  return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
+return getTokenName(Macro.Name) == Id;
+  });
+});
+  }
+}
+
 void MacroToEnumCallbacks::EndOfMainFile() {
+  invalidateExpressionNames();
+
   for (const MacroList &MacroList : Enums) {
 if (MacroList.empty())
   continue;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123889: [clang-tidy] Improve macro handling in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 423265.
LegalizeAdulthood added a comment.

Internal methods are private


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

https://reviews.llvm.org/D123889

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -181,6 +181,12 @@
 #define USE_IFDEF 1
 #define USE_IFNDEF 1
 
+// Undef'ing first and then defining later should still exclude this macro
+#undef USE_UINT64
+#define USE_UINT64 0
+#undef USE_INT64
+#define USE_INT64 0
+
 #if defined(USE_FOO) && USE_FOO
 extern void foo();
 #else
@@ -243,6 +249,27 @@
 #define IFNDEF3 3
 #endif
 
+// Macros used in conditions are invalidated, even if they look
+// like enums after they are used in conditions.
+#if DEFINED_LATER1
+#endif
+#ifdef DEFINED_LATER2
+#endif
+#ifndef DEFINED_LATER3
+#endif
+#undef DEFINED_LATER4
+#if ((defined(DEFINED_LATER5) || DEFINED_LATER6) && DEFINED_LATER7) || (DEFINED_LATER8 > 10)
+#endif
+
+#define DEFINED_LATER1 1
+#define DEFINED_LATER2 2
+#define DEFINED_LATER3 3
+#define DEFINED_LATER4 4
+#define DEFINED_LATER5 5
+#define DEFINED_LATER6 6
+#define DEFINED_LATER7 7
+#define DEFINED_LATER8 8
+
 // Sometimes an argument to ifdef can be classified as a keyword token.
 #ifdef __restrict
 #endif
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -222,6 +222,8 @@
   void conditionStart(const SourceLocation &Loc);
   void checkCondition(SourceRange ConditionRange);
   void checkName(const Token &MacroNameTok);
+  void rememberExpressionName(const Token &MacroNameTok);
+  void invalidateExpressionNames();
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
@@ -230,6 +232,7 @@
   const SourceManager &SM;
   SmallVector Enums;
   SmallVector Files;
+  std::vector ExpressionNames;
   FileState *CurrentFile = nullptr;
 };
 
@@ -284,8 +287,9 @@
 }
 
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  StringRef Id = getTokenName(MacroNameTok);
+  rememberExpressionName(MacroNameTok);
 
+  StringRef Id = getTokenName(MacroNameTok);
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
   return getTokenName(Macro.Name) == Id;
@@ -293,6 +297,14 @@
   });
 }
 
+void MacroToEnumCallbacks::rememberExpressionName(const Token &MacroNameTok) {
+  std::string Id = getTokenName(MacroNameTok).str();
+  auto Pos = llvm::lower_bound(ExpressionNames, Id);
+  if (Pos == ExpressionNames.end() || *Pos != Id) {
+ExpressionNames.insert(Pos, Id);
+  }
+}
+
 void MacroToEnumCallbacks::FileChanged(SourceLocation Loc,
FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -384,6 +396,8 @@
 void MacroToEnumCallbacks::MacroUndefined(const Token &MacroNameTok,
   const MacroDefinition &MD,
   const MacroDirective *Undef) {
+  rememberExpressionName(MacroNameTok);
+
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
 return getTokenName(Macro.Name) == getTokenName(MacroNameTok);
   };
@@ -447,7 +461,19 @@
 CurrentFile->GuardScanner = IncludeGuard::IfGuard;
 }
 
+void MacroToEnumCallbacks::invalidateExpressionNames() {
+  for (const std::string &Id : ExpressionNames) {
+llvm::erase_if(Enums, [Id](const MacroList &MacroList) {
+  return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
+return getTokenName(Macro.Name) == Id;
+  });
+});
+  }
+}
+
 void MacroToEnumCallbacks::EndOfMainFile() {
+  invalidateExpressionNames();
+
   for (const MacroList &MacroList : Enums) {
 if (MacroList.empty())
   continue;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123889: [clang-tidy] Improve macro handling in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:235
   SmallVector Files;
+  std::vector ExpressionNames;
   FileState *CurrentFile = nullptr;

aaron.ballman wrote:
> This smells expensive (compared to a `SmallVector` or somesuch).
Initially I had StringRef, but the problem is that the lifetime of those string 
references doesn't last long enough.  From StringRef docs:


> This class does not own the string data, it is expected to be used in 
> situations where the character data resides in some other buffer, whose 
> lifetime extends past that of the StringRef. For this reason, it is not in 
> general safe to store a StringRef.







Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:303-305
+  if (Pos == ExpressionNames.end() || *Pos != Id) {
+ExpressionNames.insert(Pos, Id);
+  }

aaron.ballman wrote:
> So you're manually keeping the vector sorted instead of using a set?
> 
> Any chance that we can store a `StringRef` rather than paying the expense of 
> all these copies? And would it make sense to switch to using an ordered 
> container rather than an unordered one?
Can't store a `StringRef` because the underlying data doesn't live long enough 
(I tried it).

Yes, I was keeping a sorted array.  I can switch it to a set; I guess memory 
locality doesn't really matter much in this case because we've already got 
`std::string` throwing things on the heap.

Let me know what you think is best, I'm ambivalent about it.


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

https://reviews.llvm.org/D123889

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

[PATCH] D123889: [clang-tidy] Improve macro handling in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
LegalizeAdulthood marked 2 inline comments as done.
Closed by commit rG08881c2de66b: [clang-tidy] Improve macro handling in 
modernize-macro-to-enum (authored by LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123889

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -181,6 +181,12 @@
 #define USE_IFDEF 1
 #define USE_IFNDEF 1
 
+// Undef'ing first and then defining later should still exclude this macro
+#undef USE_UINT64
+#define USE_UINT64 0
+#undef USE_INT64
+#define USE_INT64 0
+
 #if defined(USE_FOO) && USE_FOO
 extern void foo();
 #else
@@ -243,6 +249,27 @@
 #define IFNDEF3 3
 #endif
 
+// Macros used in conditions are invalidated, even if they look
+// like enums after they are used in conditions.
+#if DEFINED_LATER1
+#endif
+#ifdef DEFINED_LATER2
+#endif
+#ifndef DEFINED_LATER3
+#endif
+#undef DEFINED_LATER4
+#if ((defined(DEFINED_LATER5) || DEFINED_LATER6) && DEFINED_LATER7) || (DEFINED_LATER8 > 10)
+#endif
+
+#define DEFINED_LATER1 1
+#define DEFINED_LATER2 2
+#define DEFINED_LATER3 3
+#define DEFINED_LATER4 4
+#define DEFINED_LATER5 5
+#define DEFINED_LATER6 6
+#define DEFINED_LATER7 7
+#define DEFINED_LATER8 8
+
 // Sometimes an argument to ifdef can be classified as a keyword token.
 #ifdef __restrict
 #endif
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -222,6 +222,8 @@
   void conditionStart(const SourceLocation &Loc);
   void checkCondition(SourceRange ConditionRange);
   void checkName(const Token &MacroNameTok);
+  void rememberExpressionName(const Token &MacroNameTok);
+  void invalidateExpressionNames();
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
@@ -230,6 +232,7 @@
   const SourceManager &SM;
   SmallVector Enums;
   SmallVector Files;
+  std::vector ExpressionNames;
   FileState *CurrentFile = nullptr;
 };
 
@@ -284,8 +287,9 @@
 }
 
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  StringRef Id = getTokenName(MacroNameTok);
+  rememberExpressionName(MacroNameTok);
 
+  StringRef Id = getTokenName(MacroNameTok);
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
 return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
   return getTokenName(Macro.Name) == Id;
@@ -293,6 +297,14 @@
   });
 }
 
+void MacroToEnumCallbacks::rememberExpressionName(const Token &MacroNameTok) {
+  std::string Id = getTokenName(MacroNameTok).str();
+  auto Pos = llvm::lower_bound(ExpressionNames, Id);
+  if (Pos == ExpressionNames.end() || *Pos != Id) {
+ExpressionNames.insert(Pos, Id);
+  }
+}
+
 void MacroToEnumCallbacks::FileChanged(SourceLocation Loc,
FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -384,6 +396,8 @@
 void MacroToEnumCallbacks::MacroUndefined(const Token &MacroNameTok,
   const MacroDefinition &MD,
   const MacroDirective *Undef) {
+  rememberExpressionName(MacroNameTok);
+
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
 return getTokenName(Macro.Name) == getTokenName(MacroNameTok);
   };
@@ -447,7 +461,19 @@
 CurrentFile->GuardScanner = IncludeGuard::IfGuard;
 }
 
+void MacroToEnumCallbacks::invalidateExpressionNames() {
+  for (const std::string &Id : ExpressionNames) {
+llvm::erase_if(Enums, [Id](const MacroList &MacroList) {
+  return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
+return getTokenName(Macro.Name) == Id;
+  });
+});
+  }
+}
+
 void MacroToEnumCallbacks::EndOfMainFile() {
+  invalidateExpressionNames();
+
   for (const MacroList &MacroList : Enums) {
 if (MacroList.empty())
   continue;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124066: [clang-tidy] Ignore macros defined within declarations in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added reviewers: aaron.ballman, alexfh.
LegalizeAdulthood added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
LegalizeAdulthood requested review of this revision.

Modernize-macro-to-enum shouldn't try to convert macros to enums
when they are defined inside a declaration or definition, only
when the macros are defined at the top level.  Since preprocessing
is disconnected from AST traversal, match nodes in the AST and then
invalidate source ranges spanning AST nodes before issuing diagnostics.

ClangTidyCheck::onEndOfTranslationUnit is called before
PPCallbacks::EndOfMainFile, so defer final diagnostics to the
PPCallbacks implementation.

Fixes #54883


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124066

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum -fno-delayed-template-parsing
 // C++14 or later required for binary literals.
 
 #if 1
@@ -303,3 +303,75 @@
   FN_GREEN(0);
   FN_BLUE(0);
 }
+
+// Ignore macros defined inside a top-level function definition.
+void g(int x)
+{
+  if (x != 0) {
+#define INSIDE1 1
+#define INSIDE2 2
+if (INSIDE1 > 1) {
+  f();
+}
+  } else {
+if (INSIDE2 == 1) {
+  f();
+}
+  }
+}
+
+// Ignore macros defined inside a top-level function declaration.
+extern void g2(
+#define INSIDE3 3
+#define INSIDE4 4
+);
+
+// Ignore macros defined inside a record (structure) declaration.
+struct S {
+#define INSIDE5 5
+#define INSIDE6 6
+  char storage[INSIDE5];
+};
+class C {
+#define INSIDE7 7
+#define INSIDE8 8
+};
+template 
+#define INSIDE9 9
+bool fn()
+{
+  #define INSIDE10 10
+  return INSIDE9 > 1 || INSIDE10 < N;
+}
+extern int
+#define INSIDE11 11
+v;
+template 
+class C2 {
+public:
+#define INSIDE12 12
+char storage[N];
+  bool f() {
+return N > INSIDE12;
+  }
+  bool g();
+};
+template 
+#define INSIDE13 13
+bool C2::g() {
+#define INSIDE14 14
+  return N < INSIDE12 || N > INSIDE13 || INSIDE14 > N;
+};
+template 
+class C3 {
+  T data;
+};
+template 
+#define INSIDE15 15
+using Data = C3;
+using Data2 =
+#define INSIDE16 16
+char[INSIDE16];
+constexpr int
+#define INSIDE17 17
+value = INSIDE17;
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
@@ -15,6 +15,8 @@
 namespace tidy {
 namespace modernize {
 
+class MacroToEnumCallbacks;
+
 /// Replaces groups of related macros with an unscoped anonymous enum.
 ///
 /// For the user-facing documentation see:
@@ -25,6 +27,11 @@
   : ClangTidyCheck(Name, Context) {}
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  MacroToEnumCallbacks *PPCallback{};
 };
 
 } // namespace modernize
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -121,6 +121,8 @@
   SourceLocation LastMacroLocation;
 };
 
+} // namespace
+
 class MacroToEnumCallbacks : public PPCallbacks {
 public:
   MacroToEnumCallbacks(MacroToEnumCheck *Check, const LangOptions &LangOptions,
@@ -197,6 +199,8 @@
   // After we've seen everything, issue warnings and fix-its.
   void EndOfMainFile() override;
 
+  void invalidateRange(SourceRange Range);
+
 private:
   void newEnum() {
 if (Enums.empty() || !Enums.back().empty())
@@ -224,6 +228,7 @@
   void checkName(const Token &MacroNameTok);
   void rememberExpressionName(const Token &MacroNameTok);
   void invalidateExpressionNames();
+  void issueDiagnostics();
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
@@ -472,8 +477,20 @@
 }
 
 void MacroToEnumCallbacks::EndOfMainFile() {
-  invalidateExpressionNames();
+invalidateEx

[PATCH] D124066: [clang-tidy] Ignore macros defined within declarations in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 423815.
LegalizeAdulthood added a comment.

Add comments describing test cases


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

https://reviews.llvm.org/D124066

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum -fno-delayed-template-parsing
 // C++14 or later required for binary literals.
 
 #if 1
@@ -303,3 +303,89 @@
   FN_GREEN(0);
   FN_BLUE(0);
 }
+
+// Ignore macros defined inside a top-level function definition.
+void g(int x)
+{
+  if (x != 0) {
+#define INSIDE1 1
+#define INSIDE2 2
+if (INSIDE1 > 1) {
+  f();
+}
+  } else {
+if (INSIDE2 == 1) {
+  f();
+}
+  }
+}
+
+// Ignore macros defined inside a top-level function declaration.
+extern void g2(
+#define INSIDE3 3
+#define INSIDE4 4
+);
+
+// Ignore macros defined inside a record (structure) declaration.
+struct S {
+#define INSIDE5 5
+#define INSIDE6 6
+  char storage[INSIDE5];
+};
+class C {
+#define INSIDE7 7
+#define INSIDE8 8
+};
+
+// Ignore macros defined inside a template function definition.
+template 
+#define INSIDE9 9
+bool fn()
+{
+  #define INSIDE10 10
+  return INSIDE9 > 1 || INSIDE10 < N;
+}
+
+// Ignore macros defined inside a variable declaration.
+extern int
+#define INSIDE11 11
+v;
+
+// Ignore macros defined inside a template class definition.
+template 
+class C2 {
+public:
+#define INSIDE12 12
+char storage[N];
+  bool f() {
+return N > INSIDE12;
+  }
+  bool g();
+};
+
+// Ignore macros defined inside a template member function definition.
+template 
+#define INSIDE13 13
+bool C2::g() {
+#define INSIDE14 14
+  return N < INSIDE12 || N > INSIDE13 || INSIDE14 > N;
+};
+
+// Ignore macros defined inside a template type alias.
+template 
+class C3 {
+  T data;
+};
+template 
+#define INSIDE15 15
+using Data = C3;
+
+// Ignore macros defined inside a type alias.
+using Data2 =
+#define INSIDE16 16
+char[INSIDE16];
+
+// Ignore macros defined inside a (constexpr) variable definition.
+constexpr int
+#define INSIDE17 17
+value = INSIDE17;
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
@@ -15,6 +15,8 @@
 namespace tidy {
 namespace modernize {
 
+class MacroToEnumCallbacks;
+
 /// Replaces groups of related macros with an unscoped anonymous enum.
 ///
 /// For the user-facing documentation see:
@@ -25,6 +27,11 @@
   : ClangTidyCheck(Name, Context) {}
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  MacroToEnumCallbacks *PPCallback{};
 };
 
 } // namespace modernize
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -121,6 +121,8 @@
   SourceLocation LastMacroLocation;
 };
 
+} // namespace
+
 class MacroToEnumCallbacks : public PPCallbacks {
 public:
   MacroToEnumCallbacks(MacroToEnumCheck *Check, const LangOptions &LangOptions,
@@ -197,6 +199,8 @@
   // After we've seen everything, issue warnings and fix-its.
   void EndOfMainFile() override;
 
+  void invalidateRange(SourceRange Range);
+
 private:
   void newEnum() {
 if (Enums.empty() || !Enums.back().empty())
@@ -224,6 +228,7 @@
   void checkName(const Token &MacroNameTok);
   void rememberExpressionName(const Token &MacroNameTok);
   void invalidateExpressionNames();
+  void issueDiagnostics();
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
@@ -472,8 +477,20 @@
 }
 
 void MacroToEnumCallbacks::EndOfMainFile() {
-  invalidateExpressionNames();
+invalidateExpressionNames();
+issueDiagnostics();
+}
+
+void MacroToEnumCallbacks::invalidateRange(SourceRange Range) {
+  llvm::erase_if(Enums, [Range](const MacroList &MacroList) {
+return l

[PATCH] D124066: [clang-tidy] Ignore macros defined within declarations in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 423818.
LegalizeAdulthood added a comment.

Delete debugging aids


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

https://reviews.llvm.org/D124066

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum -fno-delayed-template-parsing
 // C++14 or later required for binary literals.
 
 #if 1
@@ -303,3 +303,89 @@
   FN_GREEN(0);
   FN_BLUE(0);
 }
+
+// Ignore macros defined inside a top-level function definition.
+void g(int x)
+{
+  if (x != 0) {
+#define INSIDE1 1
+#define INSIDE2 2
+if (INSIDE1 > 1) {
+  f();
+}
+  } else {
+if (INSIDE2 == 1) {
+  f();
+}
+  }
+}
+
+// Ignore macros defined inside a top-level function declaration.
+extern void g2(
+#define INSIDE3 3
+#define INSIDE4 4
+);
+
+// Ignore macros defined inside a record (structure) declaration.
+struct S {
+#define INSIDE5 5
+#define INSIDE6 6
+  char storage[INSIDE5];
+};
+class C {
+#define INSIDE7 7
+#define INSIDE8 8
+};
+
+// Ignore macros defined inside a template function definition.
+template 
+#define INSIDE9 9
+bool fn()
+{
+  #define INSIDE10 10
+  return INSIDE9 > 1 || INSIDE10 < N;
+}
+
+// Ignore macros defined inside a variable declaration.
+extern int
+#define INSIDE11 11
+v;
+
+// Ignore macros defined inside a template class definition.
+template 
+class C2 {
+public:
+#define INSIDE12 12
+char storage[N];
+  bool f() {
+return N > INSIDE12;
+  }
+  bool g();
+};
+
+// Ignore macros defined inside a template member function definition.
+template 
+#define INSIDE13 13
+bool C2::g() {
+#define INSIDE14 14
+  return N < INSIDE12 || N > INSIDE13 || INSIDE14 > N;
+};
+
+// Ignore macros defined inside a template type alias.
+template 
+class C3 {
+  T data;
+};
+template 
+#define INSIDE15 15
+using Data = C3;
+
+// Ignore macros defined inside a type alias.
+using Data2 =
+#define INSIDE16 16
+char[INSIDE16];
+
+// Ignore macros defined inside a (constexpr) variable definition.
+constexpr int
+#define INSIDE17 17
+value = INSIDE17;
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h
@@ -15,6 +15,8 @@
 namespace tidy {
 namespace modernize {
 
+class MacroToEnumCallbacks;
+
 /// Replaces groups of related macros with an unscoped anonymous enum.
 ///
 /// For the user-facing documentation see:
@@ -25,6 +27,11 @@
   : ClangTidyCheck(Name, Context) {}
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  MacroToEnumCallbacks *PPCallback{};
 };
 
 } // namespace modernize
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -121,6 +121,8 @@
   SourceLocation LastMacroLocation;
 };
 
+} // namespace
+
 class MacroToEnumCallbacks : public PPCallbacks {
 public:
   MacroToEnumCallbacks(MacroToEnumCheck *Check, const LangOptions &LangOptions,
@@ -197,6 +199,8 @@
   // After we've seen everything, issue warnings and fix-its.
   void EndOfMainFile() override;
 
+  void invalidateRange(SourceRange Range);
+
 private:
   void newEnum() {
 if (Enums.empty() || !Enums.back().empty())
@@ -224,6 +228,7 @@
   void checkName(const Token &MacroNameTok);
   void rememberExpressionName(const Token &MacroNameTok);
   void invalidateExpressionNames();
+  void issueDiagnostics();
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
@@ -472,8 +477,20 @@
 }
 
 void MacroToEnumCallbacks::EndOfMainFile() {
-  invalidateExpressionNames();
+invalidateExpressionNames();
+issueDiagnostics();
+}
 
+void MacroToEnumCallbacks::invalidateRange(SourceRange Range) {
+  llvm::erase_if(Enums, [Range](const MacroList &MacroList) {
+return llvm::any_of(M

[PATCH] D124066: [clang-tidy] Ignore macros defined within declarations in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D124066#3463008 , @aaron.ballman 
wrote:

> This seems like a case where we might want a configuration option (maybe). 
> [...]
> WDYT?

In my bug report on this problem, I sketch out a plan of attack:

1. **DON'T BREAK MY CODE** -- that is this review `:)`
2. Do some analysis of macro expansion locations to determine the nearest 
enclosing scope at which the enum should be declared.

https://github.com/llvm/llvm-project/issues/54883


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

https://reviews.llvm.org/D124066

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

[PATCH] D124066: [clang-tidy] Ignore macros defined within declarations in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Should probably split that bug report into 2 bugs for the two phases of attack.


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

https://reviews.llvm.org/D124066

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

[PATCH] D124066: [clang-tidy] Ignore macros defined within declarations in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

The main problem with scenario 2) is the weird interaction between when 
PPCallbacks methods are invoked and AST traversal is invoked.


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

https://reviews.llvm.org/D124066

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

[PATCH] D117306: [clang-tidy] Add new check 'shared-ptr-array-mismatch'.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SmartPtrArrayMismatchCheck.h:40
+  /// in this class.
+  virtual SmartPtrClassMatcher getSmartPointerClassMatcher() const = 0;
+

There's only one implementation of this abstract method declared here.

Why the extra level in the class hierarchy if there's only one implementation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117306

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

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D117522#3290604 , @carlosgalvezp 
wrote:

> This will conflict with `Enum.3` from `cppcoreguidelines`

I went back and looked at Enum.3 Prefer class enums over “plain” enums 
,
and implementing that is the second-pass check I discussed earlier.

This check is more about implementing Enum.1 Prefer enumerations over macros 
.

This brings up the question of whether or not there should be a cppguidelines 
alias
for this check.


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

https://reviews.llvm.org/D117522

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

[PATCH] D118104: Make run-clang-tidy.py print the configured checks correctly

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D118104#3292862 , @JesApp wrote:

> In general though, I think the script is complex enough to warrant some 
> testing.

What is the feasibility of Python unit tests Instead of a heavy-weight `lit` 
oriented test?


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

https://reviews.llvm.org/D118104

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

[PATCH] D117306: [clang-tidy] Add new check 'shared-ptr-array-mismatch'.

[Richard via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117306

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

[PATCH] D97625: fix check-clang-tools tests that fail due to Windows CRLF line endings

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Do you have commit access or do you need someone to push this change for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97625

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

[PATCH] D116577: [clang-tidy] Added "boost-use-range-based-for-loop" check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/boost/UseRangeBasedForLoopCheck.h:28
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+return LangOpts.CPlusPlus11;
+  }

LegalizeAdulthood wrote:
> This is an implicit conversion from `unsigned` to `bool`,
> an explicit comparison against `!= 0` would be preferable IMO.
> 
> However, my bigger question is what happens when someone
> runs this check with `-std=c++14` or some other later version of C++?
> 
> I looked at the header for `LangOptions` and it isn't clear whether
> the bits are set to indicate supported portions of the standard or
> if they simply reflect the command-line options used when invoking
> the compiler.
When you specify a standard on the command-line, the front end
sets all the relevant bits, so if you say `-std=c++14` then C++11 is
also set (but not C++17, C++20, etc.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116577

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

[PATCH] D118104: Make run-clang-tidy.py print the configured checks correctly

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D118104#3296467 , @salman-javed-nz 
wrote:

> Anyway, I don't want what was a drive-by comment by me baloon into a lot of 
> extra work for the author, so I will not push for it, unless others really 
> want it.

I think it's reasonable to accept this change and have the tests added as a 
follow-up change.


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

https://reviews.llvm.org/D118104

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

[PATCH] D118104: Make run-clang-tidy.py print the configured checks correctly

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D118104#3295876 , @JesApp wrote:

> In D118104#3294883 , 
> @LegalizeAdulthood wrote:
>
>> What is the feasibility of Python unit tests Instead of a heavy-weight `lit` 
>> oriented test?
>
> I don't know the code-base well enough to answer that. Is python used for 
> testing purposes anywhere else?

I'm not sure; this is a little bit unusual because there is not much production
python code in llvm/clang.  I'm actually bumping into some grief in 
`check_clang_tidy.py`
that might go easier if I were to unit test some of the python itself.

> Anyway, I'm new here, so I don't want to tell you guys how to run things,
> but: In the projects I usually work on, a fix would get merged quickly and 
> things
> like "let's write a test suite for this script" would go in the backlog. 
> What's your
> opinion on that?

In this case, I think it's fine.  Generally speaking though, the coding 
guidelines for llvm/clang
require that tests be added alongside the new features or bug fixes.  This is 
certainly the case
when we add new checks, for instance.

See https://llvm.org/docs/DeveloperPolicy.html#test-cases


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

https://reviews.llvm.org/D118104

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

[PATCH] D118927: [clang-tidy] Fix invalid fix-it for cppcoreguidelines-prefer-member-initializer

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp:251
+  if (InsertPos.isValid())
+InvalidFix |= InsertPos.isMacroID();
+  else

I'm not a fan of using bit-wise operators on booleans, but I see that
the code for this check was already doing that.
(See https://github.com/llvm/llvm-project/issues/40307)



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp:293
 << Field;
 if (!InvalidFix) {
+  StringRef NewInit = Lexer::getSourceText(

I'm not a fan of double negatives either `:)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118927

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